Bug #2413

off-by-one when parsing Connection header

Added by pmarinescu about 1 year ago. Updated about 1 year ago.

Status:FixedStart date:2012-04-17
Priority:HighDue date:
Assignee:-% Done:

100%

Category:core
Target version:1.4.31
Missing in 1.5.x:No

Description

Valgrind reports:

==21749== Invalid read of size 1
==21749==    at 0x412852: http_request_parse (request.c:246)
==21749==    by 0x40BAD8: connection_state_machine (connections.c:1428)
==21749==    by 0x407FF1: main (server.c:1507)
==21749==  Address 0x53993af is 1 bytes before a block of size 64 alloc'd
==21749==    at 0x4029B6A: malloc (vg_replace_malloc.c:263)
==21749==    by 0x41463B: buffer_prepare_copy (buffer.c:87)
==21749==    by 0x414C38: buffer_copy_string_len (buffer.c:149)
==21749==    by 0x412471: http_request_parse (request.c:808)
==21749==    by 0x40BAD8: connection_state_machine (connections.c:1428)
==21749==    by 0x407FF1: main (server.c:1507)

when a client issues a request such as 
GET / HTTP/1.0
Connection: ,whatever

The problem seems to exist in all versions.

Paul

Associated revisions

Revision 2830
Added by stbuehler about 1 year ago

Fix handling of empty header list entries in http_request_split_value, fixing invalid read in valgrind (fixes #2413)

History

#1 Updated by stbuehler about 1 year ago

  • Priority changed from Normal to High

Good catch, this looks very... broken. Although I think there won't anything bad happen as long as the byte before a string is mapped in memory (and I guess usually the malloc() allocator has some data in this place) - the value doesn't actually get used.

for (; (*end == ' ' || *end == '\t') && end > start; end--);

If start was > 0, and *start == ',', then end is start-1 and end < start - so end isn't decremented (swapping the arguments of && already fixes the memory access).

buffer_copy_string_len(ds->value, start, end-start+1);

end-start+1 is at least 0, so nothing bad should happen (I think the pointer difference is calculated as a signed value, so -1+1 shouldn't trigger an overflow)

#2 Updated by stbuehler about 1 year ago

  • Status changed from New to Fixed
  • % Done changed from 0 to 100

Applied in changeset r2830.

#3 Updated by stbuehler about 1 year ago

  • Target version changed from 1.4.x to 1.4.31

Also available in: Atom