-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add script http-cookie-flags.nse: Report insecurely set HTTP session cookie flags. #669
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great script and will be very useful for Nmap users. Take a look at the suggested changes and let us know what you think.
|
||
--- | ||
-- @usage | ||
-- nmap -p 443 --script ssl-cert-intaddr <target> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy-paste error, wrong script name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix that.
@@ -0,0 +1,139 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need local string = require "string"
here. Helpful code checking scripts are available on our Code Standards wiki page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix that and check out those scripts.
action = function(host, port) | ||
local all_issues = stdnse.output_table() | ||
|
||
all_issues['/'] = check_path(host, port, '/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of our http-*
scripts have a uri
or path
script-arg to allow requesting a specific path other than /
. Please modify this to accept the same. It may be useful to let the user pass a specific cookie name to check, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
'^FedAuth$', | ||
'^ASPXAUTH$', | ||
'[Ss][Ee][Ss][Ss][Ii][Oo][Nn]_*[Ii][Dd]' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more patterns: ^session$
is used by some frameworks, and your SESSION_ID match could use any non-alpha character in place of the underscore: [^%a]*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add those & change session_id.
without the httponly flag. Reports any session cookies set over SSL without | ||
the secure flag. If http-enum.nse is also run, any interesting paths found | ||
by it will be checked in addition to the root. | ||
]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this would be better called simply "http-cookie-flags" and have an option to check flags on all cookies. By removing "session" from the name, we open it up to other patterns (in the default case) that could specify other sensitive cookie names, even if they are not session cookies. Of course, the description would have to make it clear that only known-sensitive cookies are being reported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I will rename the script.
local all_pages = stdnse.registry_get({host.ip, 'www', port.number, 'all_pages'}) | ||
if all_pages then | ||
for _,path in ipairs(all_pages) do | ||
all_issues[path] = check_path(host, port, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry that this might result in a lot of duplicate output if a cookie is set with a path of /
for all lower-level urls. We should be able to pass the output table to the check_path function and have it add discovered cookies to the table based on the cookie's path attribute instead of simply using the path that was requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. I'll change the structure of the output table like you suggest
However, in some cases I think that could lead to confusing output if different web apps set different cookies for the same path. For example:
GET /mail HTTP/1.0
200 OK
Set-Cookie: session_id=12345; path=/
and:
GET /docs HTTP/1.0
200 OK
Set-Cookie: ASP.NET_SessionId=67890; path=/
The script's output would be:
| /:
| session_id:
| httponly flag not set
| ASP.NET_SessionId:
| httponly flag not set
What do you think about adding the requested path (from the first request the cookie was seen from, anyway) as an additional information item with the cookie, like this:
| /:
| session_id:
| httponly flag not set
| Initially set from /mail
| ASP.NET_SessionId:
| httponly flag not set
| Initially set from /docs
…o check flags on. Changed is_session_cookie() into a closure implementing desired session cookie name test.
Examines cookies set by HTTP services. Reports any session cookies set without the httponly flag. Reports any session cookies set over SSL without the secure flag. If http-enum.nse is also run, any interesting paths found by it will be checked in addition to the root.