Hi Raphael,
On Fri, Dec 16, 2016 at 11:29:00AM +0100, Raphael Hertzog wrote:
> Hi Roberto,
>
> On Thu, 15 Dec 2016, Roberto C. Sánchez wrote:
> > @@ -1704,7 +1704,7 @@
> > char path[256];
> > char* myPath = path;
> > const char* resPath = resB->fResPath;
> > - int32_t len = resB->fResPathLen;
> > + int32_t len = uprv_min(resB->fResPathLen, 256);
> >
> > while(res == RES_BOGUS && dataEntry->fParent != NULL) { /* Otherwise, we'll look in parents */
> > dataEntry = dataEntry->fParent;
> > @@ -1712,7 +1712,7 @@
> >
> > if(dataEntry->fBogus == U_ZERO_ERROR) {
> > uprv_strncpy(path, resPath, len);
> > - uprv_strcpy(path+len, inKey);
> > + uprv_strncpy(path+len, inKey, 256-len);
>
> After use of strncpy, you have no guarantee that the string is null
> terminated. You should thus add this:
> path[255] = '\0'
>
> strncpy() avoids the buffer overwrite but if it's not NULL terminated
> you might have another "read outside of buffer" problem later on.
>
Of course, I should have noticed that. Thank you for pointing it out.
> Apart from that, assuming that the underlying problem is the buffer overwrite
> with strcpy(), then your fix should be good enough, yes.
>
My reading of the upstream issue description and the exploit proof of
concept is that the underlying problem is in fact the buffer overwrite.
I will make the change you suggest and prepare the upload.
Regards,
-Roberto
--
Roberto C. Sánchez
http://people.connexer.com/~roberto
http://www.connexer.com
Attachment:
signature.asc
Description: Digital signature