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

Parsing issues with Set-Cookie header #229

Closed
nnposter opened this Issue Oct 26, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@nnposter

nnposter commented Oct 26, 2015

Library http.lua does not correctly parse certain cases of Set-Cookie header.

Case 1: Cookie Merge

Consider the following HTTP response:

...
Set-Cookie: c1=aaa
Set-Cookie: c2=bbb; expires=Sun, 25-Oct-2015 04:35:13 GMT
...

The current code parses the headers as follows:

  • Cookie:
    • name="c1"
    • value="aaa,c2=bbb"
    • expires="Sun, 25-Oct-2015 04:35:13 GMT"

Patch http-parse-set-cookie.patch corrects the behavior, producing:

  • Cookie:
    • name="c1"
    • value="aaa"
  • Cookie:
    • name="c2"
    • value="bbb"
    • expires="Sun, 25-Oct-2015 04:35:13 GMT"

The patch does the following:

  • Function parse_set_cookie() is no longer fed response.header["set-cookie"], which is a concatenation of all the cookie definitions. Instead the function is invoked on each Set-Cookie header separately, before they are joined, inside parse_header().
  • This allows removal of unnecessary outer looping construct while true do ... end from the cookie parsing code.
  • Swaps the position of functions parse_set_cookie() and parse_header() inside http.lua. This is necessary due to the moved invocation point for parse_set_cookie().

The last two changes visually inflate the patch quite a lot but a smart diff can see through it.

Case 2: Comma Splitting

Consider the following HTTP response:

...
Set-Cookie: c1=aaa; path=/bbb/ccc,ddd/eee
...

Note that this is a legitimate path. (See W3C and RFC 6265 for details.)
The current code parses the header as follows:

  • Cookie:
    • name="c1"
    • value="aaa"
    • path="=/bbb/ccc"

Patch http-parse-set-cookie-comma.patch corrects the behavior by removing special parsing of the comma from parse_set_cookie(). The parsing is not needed any more due to the function now processing only one header at a time.

Clean-up

Finally I am proposing a clean-up of parse_set_cookie() code, removing unnecessary checks like if pos <= #s and converting string functional calls to the object notation. See patch http-parse-set-cookie-cleanup.patch.

The patches are meant to be applied in this order:

  1. http-parse-set-cookie.patch
  2. http-parse-set-cookie-comma.patch
  3. http-parse-set-cookie-cleanup.patch
@dmiller-nmap

This comment has been minimized.

Show comment
Hide comment
@dmiller-nmap

dmiller-nmap commented Oct 28, 2015

@nnposter Would this work as an implementation of your patches? https://gist.github.com/dmiller-nmap/ab75df21dc43ee822bef

@dmiller-nmap dmiller-nmap added this to the Release blockers milestone Oct 28, 2015

@nnposter

This comment has been minimized.

Show comment
Hide comment
@nnposter

nnposter Oct 28, 2015

I see two modifications, compared to what I proposed:

  • Variable value is declared at the time of its first use
  • Positions of the two functions, parse_set_cookie() and parse_header(), within http.lua have been restored

I have no problem with the first one. It is cleaner this way. The reason why I did not do that is that in another patch earlier this year my late variable declaration was moved by the committing person to the beginning of the function. So I am a bit struggling to follow the coding culture of this project.

With respect to the second change, I swapped the function positions to address the following error:

nselib/http.lua:697: variable 'parse_set_cookie' is not declared

I am getting this error again with the latest modifications.

nnposter commented Oct 28, 2015

I see two modifications, compared to what I proposed:

  • Variable value is declared at the time of its first use
  • Positions of the two functions, parse_set_cookie() and parse_header(), within http.lua have been restored

I have no problem with the first one. It is cleaner this way. The reason why I did not do that is that in another patch earlier this year my late variable declaration was moved by the committing person to the beginning of the function. So I am a bit struggling to follow the coding culture of this project.

With respect to the second change, I swapped the function positions to address the following error:

nselib/http.lua:697: variable 'parse_set_cookie' is not declared

I am getting this error again with the latest modifications.

@dmiller-nmap

This comment has been minimized.

Show comment
Hide comment
@dmiller-nmap

dmiller-nmap Oct 30, 2015

Great, I fixed the undeclared variable problem by declaring it local just above parse_header, then defining it later. I only moved it to minimize the diff and make the changes clearer. I'll apply this, then. Thanks for continuing to provide great feedback and excellent improvements!

dmiller-nmap commented Oct 30, 2015

Great, I fixed the undeclared variable problem by declaring it local just above parse_header, then defining it later. I only moved it to minimize the diff and make the changes clearer. I'll apply this, then. Thanks for continuing to provide great feedback and excellent improvements!

@nmap-bot nmap-bot closed this in 5e2bb7a Nov 2, 2015

qha added a commit to qha/nmap that referenced this issue Dec 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment