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

Improper parsing of alt-svc headers without parameters #5445

Closed
Lekensteyn opened this issue May 24, 2020 · 1 comment
Closed

Improper parsing of alt-svc headers without parameters #5445

Lekensteyn opened this issue May 24, 2020 · 1 comment
Assignees
Labels

Comments

@Lekensteyn
Copy link
Member

@Lekensteyn Lekensteyn commented May 24, 2020

I did this

curl -v https://mew.org:443 -k --alt-svc alt.cache
cat alt.cache

This produces:

* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
...
< HTTP/1.1 200 OK
...
< Accept-Ranges: bytes
* Unknown alt-svc protocol "h3-28", skipping...
< Alt-Svc: h3-28=":4433",h3-27=":4433"
...
$ cat alt.cache
# Your alt-svc cache. https://curl.haxx.se/docs/alt-svc.html
# This file was generated by libcurl! Edit at your own risk.
$

I expected the following

alt.cache should contain a h3-27 entry which should have enabled h3-27 support on the next attempt.

curl/libcurl version

curl 7.71.0-DEV (Linux) libcurl/7.71.0-DEV BoringSSL zlib/1.2.11 quiche/0.3.0
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher http https imap imaps pop3 pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: alt-svc AsynchDNS HTTP3 HTTPS-proxy IPv6 Largefile libz NTLM SSL UnixSockets

Pretty much current git master (commit d75e6ce) plus a fix for the ngtcp2 build (not relevant).

operating system

Arch Linux.

Further information

It appears there is a bug in what Curl_altsvc_parse expects and what the caller provides. The function expects to work on a single header line without CRLF, this is being verified by the unit tests (tests/unit/unit1654.c).

However, the actual HTTP code passes the header including the CRLF. This means that after parsing the last part of the alt-svc header (h3-27=":4433"), it returns early because there were no more options. It did not save the new alternative service entry though...

        /* Handle the optional 'ma' and 'persist' flags. Unknown flags
           are skipped. */
        for(;;) {
          while(*p && ISBLANK(*p) && *p != ';' && *p != ',')
            p++;
          if(!*p || *p == ',')    // <-- should have triggered, but it does not because p = "\r\n"
            break;
          p++; /* pass the semicolon */
          if(!*p)
            break;
          result = getalnum(&p, option, sizeof(option));
@jay
Copy link
Member

@jay jay commented May 25, 2020

while(*p && ISBLANK(*p) && *p != ';' && *p != ',')

why not just while(ISBLANK(*p))?

and if a semicolon is required after that I think the check should be explicit

@jay jay added the HTTP/3 label May 25, 2020
bagder added a commit that referenced this issue May 25, 2020
Fixed the alt-svc parser to treat a newline as end of line.

The unit tests in test 1654 were done without CRLF and thus didn't quite
match the real world. Now they use CRLF as well.

Reported-by: Peter Wu
Fixes #5445
@bagder bagder self-assigned this May 25, 2020
bagder added a commit that referenced this issue May 25, 2020
Fixed the alt-svc parser to treat a newline as end of line.

The unit tests in test 1654 were done without CRLF and thus didn't quite
match the real world. Now they use CRLF as well.

Reported-by: Peter Wu
Assisted-by: Peter Wu
Assisted-by: Jay Satiro
Fixes #5445
Closes #5446
bagder added a commit that referenced this issue May 25, 2020
Fixed the alt-svc parser to treat a newline as end of line.

The unit tests in test 1654 were done without CRLF and thus didn't quite
match the real world. Now they use CRLF as well.

Reported-by: Peter Wu
Assisted-by: Peter Wu
Assisted-by: Jay Satiro
Fixes #5445
Closes #5446
@bagder bagder closed this in d844f2b May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.