Skip to content

bound parameter name lookahead to the body in parse_param_name - #4460

Open
Sahana2524 wants to merge 1 commit into
OpenSIPS:masterfrom
Sahana2524:digest-param-name-bound
Open

Sahana2524 wants to merge 1 commit into
OpenSIPS:masterfrom
Sahana2524:digest-param-name-bound

Conversation

@Sahana2524

Copy link
Copy Markdown
Contributor

Summary
An Authorization / Proxy-Authorization Digest header whose body ends on a short or value-less parameter makes parse_param_name() read up to 4 bytes past the header body.

Details
parse_digest_cred() hands each parameter to parse_param_name() in parser/digest/param_parser.c, with _s running to the end of the header body. Two reads there are not bounded by _s->len:

	p = _s->s;
	val = READ(p);            /* p[0..3], before the length check */

	if (_s->len < 4) {
		goto other;
	}
	...
 end:
	...
	trim_leading(_s);
	if (_s->s[0] == '=') {    /* _s->len can be 0 here */

The first reads a 4-byte word before checking that 4 bytes are left, so a trailing parameter shorter than that (Digest a), or the empty remainder after Digest or a trailing , (Digest uri=x,), is read past the end. The second one is hit when a recognized name (realm, uri, nonce, ...) is the last thing in the body with no = after it (Digest realm): trim_leading() leaves len == 0 and s[0] is the byte after the body.

The header is parsed before anything is authenticated (pre_auth() via find_credentials(), the $au / $ar / $adu variables, dispatcher, call_control, sipcapture, msrp_relay), so the bytes are fully controlled by the sender. Inside a SIP message what follows is usually the header's CRLF, which is likely why this has gone unnoticed, but the parser is still reading memory outside the string it was given.

Running parse_digest_cred() through the same loop as parser/test/test_oob.c (every prefix of a few credential strings placed against a guard page, in both directions) gave 91 faults in 502 runs on the current code and none with the patch.

Solution
The READ(p) now happens after the _s->len < 4 check (short names already go to other, which only scans the bytes that are there), and the = test requires _s->len > 0. A name with no value still falls through to other, finds no = and is rejected exactly as before.

A new parser/test/test_parse_digest.c checks the parsed fields for valid credentials and the rejection of value-less ones, and runs the truncation set through the guard-page harness. It segfaults on the current tree and passes all 509 checks with the fix.

Compatibility
No behavior change for well-formed credentials: every field, plus the parsed algorithm and qop, comes out the same. Bodies ending on a bare name were already rejected (-2) and still are, just without the over-read.

Closing issues
None.

parse_param_name() read a 4-byte word from the parameter before checking
that 4 bytes were left, and after recognizing a parameter name it tested
the next byte for '=' without checking that the body had any bytes left.
A Digest credentials body ending on a short or value-less parameter
(e.g. "Digest a", "Digest realm") or right after a ',' made it read up
to 4 bytes past the Authorization / Proxy-Authorization header body.

Do the word read only after the length check and require a byte to be
left before looking for '='. Add a parser test that runs the credentials
parser through the guard-page harness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant