Comment 17 for bug 308060

On Fri, Mar 20, 2009 at 02:09:33PM -0000, Aurélien Gâteau wrote:
> http://launchpadlibrarian.net/24149888/02-avoid_potential_buffer_overrun.diff

"n" should be verified to be !=0, so that result[-1] isn't used.

See the notes from "man strcpy":

       If there is no terminating null byte in the first n characters of src,
       strncpy() produces an unterminated string in dest. Programmers often
       prevent this mistake by forcing termination as follows:

           strncpy(buf, str, n);
           if (n > 0)
               buf[n - 1]= '\0';

This change does not have the same functionality (in toXMLStringUnSafe):

- case 4: *(dest++)=*(source++);
- case 3: *(dest++)=*(source++);
- case 2: *(dest++)=*(source++);
- case 1: *(dest++)=*(source++);
+ case 4:
+ case 3:
+ case 2:
+ case 1:
+ *(dest++)=*(source++);
+ length--;
+ break;

the original can copy up to 4 characters (notice the lack of breaks).

Also, nothing in the loop is testing that "length" is remaining positive.
This should be checked in toXMLStringUnSafe, CreateXMLStringR (if nResult
is ever >= length, abort), and in _tcscpy (where it should abort if n<0).
(Note that strncpy takes size_t, not int, so a signed to unsigned
conversion would take place -- best to check the int before getting to
the strncpy call.)

--
Kees Cook
Ubuntu Security Team