Skip to content

Set-Cookie parser clobbers cookie name/value when HttpOnly/Secure present (index confusion between cft[] and lws_cookie_elements) #3652

Description

@AlexMath23

Summary

lws_parse_set_cookie() in lib/roles/http/cookie.c overwrites the
already-parsed cookie name and/or value with the literal string
"T" whenever the Set-Cookie header contains the HttpOnly and/or
Secure attributes. This is not cosmetic: the corrupted name/value is
what gets persisted to the NSC cookie jar and re-sent on subsequent
requests, so any cookie using these (extremely common) attributes is
effectively unusable.

Root cause

lws_parse_set_cookie() (lib/roles/http/cookie.c) uses a single loop
variable n to index into two arrays with different, incompatible
orderings
:

  1. cft[] — a lookup table used only to identify which cookie
    attribute matched a given ; xxx fragment:
   static struct {
       const char *const name;
       uint8_t len;
   } cft[] = {
       { "domain=",  7 },   // n == 0
       { "path=",    5 },   // n == 1
       { "expires=", 8 },   // n == 2
       { "max-age=", 8 },   // n == 3
       { "httponly", 8 },   // n == 4
       { "secure",   6 },   // n == 5
   };
  1. enum lws_cookie_elements — the indices used for c.f[] / c.l[]
    in struct lws_cookie:
   enum lws_cookie_elements {
       CE_DOMAIN,   // 0
       CE_PATH,     // 1
       CE_EXPIRES,  // 2
       CE_MAXAGE,   // 3
       CE_NAME,     // 4   <-- collides with cft[4] == "httponly"
       CE_VALUE,    // 5   <-- collides with cft[5] == "secure"

       CE_HOSTONLY, // 6
       CE_SECURE,   // 7

       CE_COUNT
   };

In the attribute-matching branch:

if (n == 4 || n == 5) {
    c.f[n] = "T";
    c.l[n] = 1;
    break;
}

When the httponly attribute matches (n == 4), this writes into
c.f[4], which per lws_cookie_elements is CE_NAME — clobbering the
name that was already parsed earlier in the same loop. When secure
matches (n == 5), it writes into c.f[5], i.e. CE_VALUE. The
intended targets, CE_HOSTONLY (6) and CE_SECURE (7), are never
touched by this branch at all.

Effect

Set-Cookie attributes present Result
none name/value correct
SameSite=... only (not in cft[]) name/value correct
HttpOnly name replaced with "T"
Secure value replaced with "T"
Secure; HttpOnly (the common case for real session cookies) both name and value replaced with "T"

The resulting garbage line does get written all the way through to the
NSC file via lws_cookie_write_nsc(), e.g.:

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions