CWE-016 - Adds MaxRequestLength check for ASP.NET#2200
CWE-016 - Adds MaxRequestLength check for ASP.NET#2200cldrn wants to merge 18 commits intogithub:mainfrom cldrn:cwe-016
Conversation
cldrn
commented
Oct 24, 2019
cldrn
commented
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
Thank you very much for your contribution, much appreciated. I have a few tedious comments, otherwise LGTM.
csharp/ql/src/Security Features/CWE-016/ASPNetMaxRequestLength.qhelp
Outdated
Show resolved
Hide resolved
csharp/ql/src/Security Features/CWE-016/ASPNetMaxRequestLength.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/Security Features/CWE-016/ASPNetMaxRequestLength.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/Security Features/CWE-016/ASPNetMaxRequestLength.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/Security Features/CWE-016/ASPNetMaxRequestLength.ql
Outdated
Show resolved
Hide resolved
csharp/ql/src/Security Features/CWE-016/ASPNetMaxRequestLength.ql
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback. I've address all your comments. I wrote them while analyzing my first project so I'm still reading up on the documentation. Awesome work! |
| @@ -0,0 +1,23 @@ | |||
| /** | |||
| * @name Large 'maxRequestLength' value | |||
| * @description Setting a large 'maxRequestLength' value may render a web page vulnerable to | |||
There was a problem hiding this comment.
| * @description Setting a large 'maxRequestLength' value may render a web page vulnerable to | |
| * @description Setting a large 'maxRequestLength' value may render a webpage vulnerable to |
| <sample src="Web.config.bad" /> | ||
|
|
||
| <p> | ||
| Unless such a high value is strictly needed, it is better to set a lower value, for example 4096: |
There was a problem hiding this comment.
Suggest using consistent units (KB) in the examples.
| <recommendation> | ||
|
|
||
| <p> | ||
| The recommended value is 4096 but you should try setting it as small as possible according to business requirements. |
There was a problem hiding this comment.
Perhaps change this sentence to:
The recommended value is 4096 KB, but you should set it as small as possible according to business requirements.
(Note, include units)
| | **Query** | **Tags** | **Purpose** | | ||
| |-----------------------------|-----------|--------------------------------------------------------------------| | ||
| | Unsafe year argument for 'DateTime' constructor (`cs/unsafe-year-construction`) | reliability, date-time | Finds incorrect manipulation of `DateTime` values, which could lead to invalid dates. | | ||
| | Large 'maxRequestLength' value (`cs/web/large-max-request-length`) | security, frameworks/asp.net, external/cwe/cwe-16 | Finds `web.config` files with large (greater than 4096) `maxRequestLength` attributes. | |
There was a problem hiding this comment.
| | Large 'maxRequestLength' value (`cs/web/large-max-request-length`) | security, frameworks/asp.net, external/cwe/cwe-16 | Finds `web.config` files with large (greater than 4096) `maxRequestLength` attributes. | | |
| | Large 'maxRequestLength' value (`cs/web/large-max-request-length`) | security, frameworks/asp.net, external/cwe/cwe-16 | Finds `web.config` files with large (greater than 4096 KB) `maxRequestLength` attributes. | |
| <sample src="Web.config.good" /> | ||
|
|
||
| </example> | ||
|
|
There was a problem hiding this comment.
Is there a reference to some authoritative docs you could include here?
|
Closed this issue as I removed that local copy and started doing each PR in their own branch. At this point I have deleted the original branch for that PR. I've addressed the comments in there but can we keep working on this one #2355? The other fork had a PR on master branch which became a mess when I added others there by mistake. Hence, why I started from stratch with each PR in their own branch. :) |
|
Closing in favor of #2355. |
3 participants
