GNOME Bugzilla – Bug 792173
there is no limit on header size and this can be use for a DOS
Last modified: 2018-02-09 14:46:09 UTC
Created attachment 366248 [details] [review] patch to include limit there is no limit on header size and this can be use for a DOS. I attach a patch for that
Review of attachment 366248 [details] [review]: This seems to be a reasonable concern. It would be good if we could also add a test case (probably to tests/header-parsing.c). ::: libsoup/soup-message-io.c @@ +210,3 @@ } +#define LIMITHEADERSIZE (64*1024) It should probably be HEADER_SIZE_LIMIT. Use spaces around * and drop the parentheses. Also, please define it at the beginning of the file, together with the RESPONSE_BLOCK_SIZE definition. @@ +264,3 @@ + _("Header too big")); + return FALSE; + } Please fix the indentation. I'm also not sure if this is the best place for that check and whether SOUP_STATUS_MALFORMED is the proper status. Maybe Dan can chime in here.
(In reply to Claudio Saavedra from comment #1) > +#define LIMITHEADERSIZE (64*1024) > > It should probably be HEADER_SIZE_LIMIT. Use spaces around * and drop the > parentheses. Agree about the name and spaces, but keep the parentheses to guarantee no operator precedence problems. > Please fix the indentation. I'm also not sure if this is the best place for > that check and whether SOUP_STATUS_MALFORMED is the proper status. Maybe Dan > can chime in here. SOUP_STATUS_MALFORMED is consistent with other errors (eg, header truncation just above). And the check basically has to be inside that loop where it is here.
Created attachment 366412 [details] [review] change define name and indentation
I add a new patch with indentation fix. I'm not able to add a test in tests/header-parsing.c because I think my fix does not involve header parsing because I prevent to start parsing too big header before.
You're right. Perhaps in misc-test.c? You can add a new path to the server that returns a long enough header and then check in the client that it is handled as expected.
Created attachment 367982 [details] [review] patch to include limit on max header size received
Created attachment 367983 [details] [review] test for max header size
Hi, Hi add my patch rebase on master plus the test to check if the patch is working. I also test the test (that fail) if applyied alone on master. I hope that this is ok to be merged on stable release
Review of attachment 367983 [details] [review]: In the future, please add the tests to the same patch that is being tested. There's no need for separate patches (it's also trivial this way to remove the test in case the patch needs to be reverted). ::: tests/misc-test.c @@ +122,3 @@ +/* Host header handling: client must be able to override the default + * value, server must be able to recognize different Host values. This comment seems copy-pasted from another test. Please change to reflect what this test does. @@ +136,3 @@ + + msg = soup_message_new_from_uri ("GET", base_uri); + SoupSession *session; Please fix the spacing. @@ +138,3 @@ + for (i=0; i<2048; i++) { + char *key = g_strdup_printf("test-long-header-key%d", i); + int i; Same in these two. @@ +141,3 @@ + soup_message_headers_append (msg->request_headers, key, value); + g_free(value); + And here.
Review of attachment 367982 [details] [review]: Since there were some things to fix in the other patch, please merge both into one. This one is ready though.
Created attachment 368098 [details] [review] patch and test
done
Review of attachment 368098 [details] [review]: You didn't fix the coding style. ::: tests/misc-test.c @@ +122,3 @@ +/* request with too big header has to be discard: request with too big header + * has to be close from server side. Requests with too big headers should be discarded with a IO error to prevent DOS attacks. @@ +136,3 @@ + + msg = soup_message_new_from_uri ("GET", base_uri); + SoupSession *session; This is still wrong. Leave a space around the equal and less-than signs (i = 0; i < 2040; i++). @@ +138,3 @@ + for (i=0; i<2048; i++) { + char *key = g_strdup_printf("test-long-header-key%d", i); + int i; Spacing is still wrong here. Always leave a space between the function name and opening parenthesis. @@ +141,3 @@ + soup_message_headers_append (msg->request_headers, key, value); + g_free(value); + Spacing is still wrong here. Always leave a space between the function name and opening parenthesis.
Created attachment 368143 [details] [review] limit maximum header size (plus test)
Review of attachment 368143 [details] [review]: Thanks, please push.
If you don't have a git.gnome.org account please let me know and I'll push it for you.
I don't have any account on git.gnome.org, I will be happy if you can push it for me. thanks
Review of attachment 368143 [details] [review]: Committed as f33de90ae.
This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.