Bug 2396 - Out of bounds read when parsing EscapeChar configuration value
Summary: Out of bounds read when parsing EscapeChar configuration value
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 6.8p1
Hardware: All All
: P5 minor
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_6_9
  Show dependency treegraph
 
Reported: 2015-05-09 20:23 AEST by Jaak Ristioja
Modified: 2015-08-11 23:03 AEST (History)
2 users (show)

See Also:


Attachments
reorder EscapeChar tests to avoid 1-byte OOB read (769 bytes, patch)
2015-05-22 14:33 AEST, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaak Ristioja 2015-05-09 20:23:58 AEST
An out of bounds memory read occurs during parsing the value for EscapeChar in the following if-statement in readconf.c:1239:

  if (arg[0] == '^' && arg[2] == 0 &&
      (u_char) arg[1] >= 64 && (u_char) arg[1] < 128)

This is erroneous because arg[2] might be one character off the end of the string. I suggest the first two branches be rewritten as follows:

  if (arg[1] == 0) // was "else if (strlen(arg) == 1)"
      value = (u_char) arg[0];
  else if (arg[0] == '^' && arg[2] == 0 &&
      (u_char) arg[1] >= 64 && (u_char) arg[1] < 128)
      value = (u_char) arg[1] & 31;

This ensures that all single-character values are handled correctly and arg[2] refers to accessible memory.



PS: As an unrelated comment I wish to mention that running ssh through valgrind's memcheck seems to yield lots of results.
Comment 1 Damien Miller 2015-05-11 12:57:09 AEST
(In reply to Jaak Ristioja from comment #0)

> PS: As an unrelated comment I wish to mention that running ssh
> through valgrind's memcheck seems to yield lots of results.

There are some leaks, but there shouldn't be any memory faults. Please report any that you see - we fix all memory errors that we see.

NB. OpenSSL's AES-NI support causes a lot of false positives under valgrind - you might have to disable AES-NI using:

export OPENSSL_ia32cap="~0x200000200000000"
Comment 2 Damien Miller 2015-05-22 14:33:19 AEST
Created attachment 2628 [details]
reorder EscapeChar tests to avoid 1-byte OOB read
Comment 3 Damien Miller 2015-05-22 14:46:23 AEST
applied - this will be in OpenSSH 6.9, thanks!
Comment 4 Damien Miller 2015-08-11 23:03:40 AEST
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1