Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarification needed for 12.4.1 #1087

Closed
elarlang opened this issue Oct 23, 2021 · 41 comments · Fixed by #2107
Closed

clarification needed for 12.4.1 #1087

elarlang opened this issue Oct 23, 2021 · 41 comments · Fixed by #2107
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review V12 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

Current V12.4.1:

Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions.

"spin-off" issue from #1065 (comment)

I'm not too happy with this requirement. It feels a bit from old-style PHP crappy architecture, where file in public folder leads to RCE.

The main question is, should we just blindly disallow to store files in public folder in general, or make it a bit more flexible.

  • sometimes files are expected to stay in public folder, like "public" images (or we ask them to serve from another domain? is it reasonable?)
  • even if .php file is stored in public folder for "PHP application", it's important to avoid to execute it as PHP program code (by web-server configuration for example)
@elarlang elarlang self-assigned this Oct 23, 2021
@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Oct 23, 2021
@jmanico
Copy link
Member

jmanico commented Oct 23, 2021

I agree this needs work. Again, I like to delete confusing requirements. We do not need to be a complete standard, we need to be a clear one, is my opinion.

@cmlh
Copy link
Contributor

cmlh commented Oct 23, 2021

We do not need to be a complete standard, we need to be a clear one, is my opinion.

Perhaps inserting an explanatory note would resolve the confusion @jmanico or something like the format below:

  1. Boardline Requirement 1
    1a. Conflicting Requirement 1a
    1b. Conflicting Requirement 1b
    1c. ...

@jmanico
Copy link
Member

jmanico commented Oct 24, 2021

This is really old-school thinking. The situation is way more complex, I again suggest we just delete this requirement and move on.

@elarlang
Copy link
Collaborator Author

Take it as a placeholder, I'll come back to this one when looking entire V12.*

@jmanico
Copy link
Member

jmanico commented Dec 3, 2021

I'd still like to delete it.

@elarlang
Copy link
Collaborator Author

elarlang commented Dec 3, 2021

proposal, delete as duplicate of 4.1.3:
V4.1.3 Verify that the principle of least privilege exists - users should only be able to access functions, data files, URLs, controllers, services, and other resources, for which they possess specific authorization. This implies protection against spoofing and elevation of privilege.

@elarlang elarlang added the 4) proposal for review Issue contains clear proposal for add/change something label Dec 3, 2021
@jmanico
Copy link
Member

jmanico commented Dec 6, 2021

I agree with your proposal.

@elarlang
Copy link
Collaborator Author

elarlang commented Dec 6, 2021

12.4.1 is removed as duplicate of 4.1.3 (just one special case of authorization problems)

For fixing "decision time situation":

# Description L1 L2 L3 CWE
4.1.3 Verify that the principle of least privilege exists - users should only be able to access functions, data files, URLs, controllers, services, and other resources, for which they possess specific authorization. This implies protection against spoofing and elevation of privilege. (C7) 285
12.4.1 Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions. 552

@jmanico jmanico closed this as completed in a281ef6 Dec 7, 2021
jmanico added a commit that referenced this issue Dec 7, 2021
removed 12.4.1 as duplicate of 4.1.3, closes #1087
@tghosth tghosth reopened this May 30, 2023
@tghosth
Copy link
Collaborator

tghosth commented May 30, 2023

Rise!GIF

@tghosth
Copy link
Collaborator

tghosth commented May 30, 2023

Sorry, @elarlang / @jmanico,

I am reopening this as I really don't agree that it is a duplicate of 4.1.3 and I don't think it should have been deleted.

This is the history of the requirement:

# Description L1 L2 L3
12.4.1 Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions. 552
12.4.1 [MODIFIED] Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions. 552
12.4.1 Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions, preferably with strong validation. 922
16.4.1 Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions, preferably with strong validation. 922
16.6 Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions, preferably with strong validation. 3.0

Basically it comes from a 3.0 requirement and I agree that the permissions/validation part is weird and that the webroot part is a little 90s but I still think the "web root" part is important and I don't want it to get lost.

I would propose the following, (note the updated CWE):

# Description L1 L2 L3 CWE
12.4.1 [MODIFIED] Verify that files obtained from untrusted sources are stored outside the web root so that there is no risk of accidental execution. 553

@elarlang @jmanico what do you think?

@elarlang
Copy link
Collaborator Author

Well, not that easy.

[MODIFIED] Verify that files obtained from untrusted sources are stored outside the web root so that there is no risk of accidental execution.

Authorization

12.4 is file storage category and was removed from that perspective.

From file execution perspective, it should be 12.3 requirement.

Client-Side execution

What about public content - like images, profile pictures, logos, some pdf files etc?

We have it kind of covered with othe requirement.

# Description L1 L2 L3 CWE
1.12.2 [MODIFIED] Verify that user-uploaded files - if required to be displayed or downloaded from the application - are served by either octet stream downloads, or from an unrelated domain, such as a cloud file storage bucket. 646
12.5.2 Verify that direct requests to uploaded files will never be executed as HTML/JavaScript content. 434

I have made proposal to merge them: #1406

It would be wasting resources to serve static images via program code and may actually cause good DoS vectors.

But those are more client-side oriented.

Server-Side execution

Now the question to solve is - why it may happen, that users can upload as "public content" something, which is executed on the server side via HTTP request as program code.

I would call it business logic problem.

# Description L1 L2 L3 CWE
12.2.1 Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content. 434

See also opened related issues: #1604

We also have another requirement to play with.

# Description L1 L2 L3 CWE
14.3.6 [GRAMMAR, MOVED FROM 12.5.1] Verify that the web tier is configured to serve only files with specific file extensions to prevent unintentional information and source code leakage. For example, backup files (e.g. .bak), temporary working files (e.g. .swp), compressed files (.zip, .tar.gz, etc.) and other extensions commonly used by editors should be blocked unless required. 552

What we clearly don't have is (in whatever wording): Verify that files obtained from untrusted sources are not executed as program code when directly accessed with HTTP request.

@tghosth tghosth added _5.0 - prep This needs to be addressed to prepare 5.0 4b Major-rework These issues need to be part of a full chapter rework V12 and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 4) proposal for review Issue contains clear proposal for add/change something labels May 30, 2023
@tghosth
Copy link
Collaborator

tghosth commented May 30, 2023

Ok, I am going to chalk this up for the rework of 12

@elarlang elarlang assigned tghosth and unassigned elarlang Oct 30, 2023
@jmanico
Copy link
Member

jmanico commented Jul 29, 2024

12.4.1 [DELETED, DUPLICATE OF 4.1.3]

Can we close this out for now and move these issues to a new issue if needed?

@jmanico jmanico closed this as completed Jul 29, 2024
@jmanico
Copy link
Member

jmanico commented Sep 13, 2024 via email

@elarlang
Copy link
Collaborator Author

What and why is going to execute it?

@jmanico
Copy link
Member

jmanico commented Sep 13, 2024 via email

@elarlang
Copy link
Collaborator Author

elarlang commented Sep 13, 2024

Kind of "here we go again" with demagogy.

If you allow user to upload (edit: a php) file to public folder that is executed when directly accessed with an HTTP request, it does not require to have "executable" permissions. Read-only is enough.

And if it comes to "executable", then something - some process must execute it. This was my question, what process and why it can execute the file?

@jmanico
Copy link
Member

jmanico commented Sep 13, 2024 via email

@elarlang
Copy link
Collaborator Author

For the mirror.

So are you suggesting that staging up uploaded files in a public directory with file permissions being set to executable to be an acceptable practice? Really?

Setting piblically available uploaded files to be read only (depending on the server and other factors) shuts down:

Sorry, but I would say that you don't know what you are talking about. For example, LFI only requires reading permission.

@jmanico
Copy link
Member

jmanico commented Sep 13, 2024 via email

@elarlang
Copy link
Collaborator Author

Let's come back to this one.

So are you suggesting that staging up uploaded files in a public directory with file permissions being set to executable to be an acceptable practice? Really?

Did I say anything towards that? No. It is your attempt to "assign" this to me, and then saying I'm attacking you :)

There is no reason to add executable permissions to uploaded files, but only removing executable permissions from files that are stored in public folders, but otherwise being directly handled by the web server (classical php solution), do not solve ANY of your listed problems. So, your proposed solution does not solve the problem (avoid file getting executed) and I think my conclusion out of that is aligned.

@jmanico
Copy link
Member

jmanico commented Sep 13, 2024 via email

@elarlang
Copy link
Collaborator Author

I think the answer was already in quoted text:

There is no reason to add executable permissions to uploaded files, but only removing executable permissions from files that are stored in public folders, /.../, do not solve ANY of your listed problems.

@tghosth
Copy link
Collaborator

tghosth commented Sep 15, 2024

So I feel like this thread got a bit confused.

@elarlang suggested above the following requirement (I made a minor wording change):

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as program code when accessed directly by an end user.

@jmanico is there anything missing from that from your perspective?

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet and removed 2) Awaiting response Awaiting a response from the original poster labels Sep 15, 2024
@tghosth tghosth removed the next meeting Filter for leaders label Sep 18, 2024
@jmanico
Copy link
Member

jmanico commented Sep 18, 2024 via email

@elarlang elarlang added the 4) proposal for review Issue contains clear proposal for add/change something label Sep 19, 2024
@elarlang
Copy link
Collaborator Author

One may argue that it disallows executing client-side javascript, so maybe we limit it to the server-side program code?

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as server-side program code when accessed directly by an end user.

@jmanico
Copy link
Member

jmanico commented Sep 20, 2024

I like your suggestion even better, @elarlang - nice ending to this. Thank you.

@tghosth
Copy link
Collaborator

tghosth commented Sep 22, 2024

One may argue that it disallows executing client-side javascript, so maybe we limit it to the server-side program code?

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as server-side program code when accessed directly by an end user.

Great, let's get this PRed in :) @elarlang

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed 4) proposal for review Issue contains clear proposal for add/change something labels Sep 22, 2024
elarlang added a commit that referenced this issue Sep 22, 2024
@elarlang elarlang added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Sep 22, 2024
elarlang added a commit that referenced this issue Sep 23, 2024
@randomstuff
Copy link
Contributor

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as server-side program code when accessed directly by an end user.

One may argue that it disallows executing client-side javascript, so maybe we limit it to the server-side program code?

Actually I understood it mainly the other way :) i.e. if the user uploads an HML or SVG file, you want to make sure that this application won't get executed on the client-side (browser).

This is a typical vector for client-side JS injection for example when you let users upload images:

  1. malicious user uploads SVG file containing malicious JavaScript code;
  2. the malicious SVG is exposed/available in the application origin;
  3. malicious user redirects you to this SVG file;
  4. SVG file is executed in your browser (including JavaScript !) and an use your session.

@elarlang
Copy link
Collaborator Author

elarlang commented Sep 24, 2024

For this scenario we have those:

V50.5 Unintended Content Interpretation

# Description L1 L2 L3 CWE
50.5.1 [GRAMMAR, MOVED FROM 12.5.2] Verify that direct requests to uploaded files will never be executed as HTML and JavaScript content. 434
50.5.2 [MODIFIED, MOVED FROM 1.12.2] Verify that user-uploaded files - if required to be displayed or downloaded from the application - are served by either octet stream downloads, or from an unrelated domain, such as a cloud file storage bucket. 646
50.5.3 [ADDED, DEPRECATES 14.4.2] Verify that security controls are in place to prevent browsers from rendering content or functionality in HTTP responses in an incorrect context (e.g., when an API or other resource is loaded directly). Possible controls could include: not serving the content unless headers indicate it is the correct context, Content-Security-Policy: sandbox, Content-Disposition: attachment, etc.

Related issue to merge 50.5.1 and 50.5.2: #1406

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 4b Major-rework These issues need to be part of a full chapter rework 6) PR awaiting review V12 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants