Thread: contrib/ buffer paranoia
The attached patch changes most of the usages of sprintf() to snprintf() in contrib/. I didn't touch the places where pointer arithmatic was being used, or other areas where the fix wasn't trivial. I would think that few, if any, of the usages of sprintf() were actually exploitable, but it's probably better to be paranoid... Unless anyone sees a problem, please apply. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
Neil Conway dijo: > The attached patch changes most of the usages of sprintf() to > snprintf() in contrib/. I didn't touch the places where pointer > arithmatic was being used, or other areas where the fix wasn't > trivial. I would think that few, if any, of the usages of sprintf() > were actually exploitable, but it's probably better to be paranoid... > > Unless anyone sees a problem, please apply. I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11 (snprintf counts the \0 at the end). -- Alvaro Herrera (<alvherre[a]atentus.com>) "Coge la flor que hoy nace alegre, ufana. Quién sabe si nacera otra mañana?"
Alvaro Herrera <alvherre@atentus.com> writes: > I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11 > (snprintf counts the \0 at the end). Yes, but so does the array declaration itself: a char[10] can hold at most 9 characters plus the '\0' terminator. I think the original code is buggy: if the author wants to store 10 characters plus a terminator in the array, it should be declared as a char[11]. Using snprintf() of length 11 with a char[10] would allow for a one-character overrun. (Or did I not drink enough coffee this morning? :-) ) Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Neil Conway dijo: > Alvaro Herrera <alvherre@atentus.com> writes: > > I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11 > > (snprintf counts the \0 at the end). > > Yes, but so does the array declaration itself: a char[10] can hold at > most 9 characters plus the '\0' terminator. I think the original code > is buggy: if the author wants to store 10 characters plus a terminator > in the array, it should be declared as a char[11]. Using snprintf() of > length 11 with a char[10] would allow for a one-character overrun. I agree. Maybe it worked out of pure luck (or some alignment magic). But while you're at it, you can as well correct the bug. -- Alvaro Herrera (<alvherre[a]atentus.com>) "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
Alvaro Herrera <alvherre@atentus.com> writes: > Neil Conway dijo: > > Alvaro Herrera <alvherre@atentus.com> writes: > > > I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11 > > > (snprintf counts the \0 at the end). > > > > Yes, but so does the array declaration itself: a char[10] can hold at > > most 9 characters plus the '\0' terminator. I think the original code > > is buggy: if the author wants to store 10 characters plus a terminator > > in the array, it should be declared as a char[11]. Using snprintf() of > > length 11 with a char[10] would allow for a one-character overrun. > > I agree. Maybe it worked out of pure luck (or some alignment magic). > But while you're at it, you can as well correct the bug. Ok, a revised patch is attached that fixes the off-by-one bug in dbase/dbf2pg.c Thanks for the code review. Cheers, Neil -- Neil Conway <neilconway@rogers.com> PGP Key ID: DB3C29FC
Attachment
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> Alvaro Herrera <alvherre@atentus.com> writes:
> > Neil Conway dijo:
> > > Alvaro Herrera <alvherre@atentus.com> writes:
> > > > I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11
> > > > (snprintf counts the \0 at the end).
> > >
> > > Yes, but so does the array declaration itself: a char[10] can hold at
> > > most 9 characters plus the '\0' terminator. I think the original code
> > > is buggy: if the author wants to store 10 characters plus a terminator
> > > in the array, it should be declared as a char[11]. Using snprintf() of
> > > length 11 with a char[10] would allow for a one-character overrun.
> >
> > I agree. Maybe it worked out of pure luck (or some alignment magic).
> > But while you're at it, you can as well correct the bug.
>
> Ok, a revised patch is attached that fixes the off-by-one bug in
> dbase/dbf2pg.c
>
> Thanks for the code review.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilconway@rogers.com>
> PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. --------------------------------------------------------------------------- Neil Conway wrote: > Alvaro Herrera <alvherre@atentus.com> writes: > > Neil Conway dijo: > > > Alvaro Herrera <alvherre@atentus.com> writes: > > > > I think in dbase/dbf2pg.c the limit of 10 to pgdate should be 11 > > > > (snprintf counts the \0 at the end). > > > > > > Yes, but so does the array declaration itself: a char[10] can hold at > > > most 9 characters plus the '\0' terminator. I think the original code > > > is buggy: if the author wants to store 10 characters plus a terminator > > > in the array, it should be declared as a char[11]. Using snprintf() of > > > length 11 with a char[10] would allow for a one-character overrun. > > > > I agree. Maybe it worked out of pure luck (or some alignment magic). > > But while you're at it, you can as well correct the bug. > > Ok, a revised patch is attached that fixes the off-by-one bug in > dbase/dbf2pg.c > > Thanks for the code review. > > Cheers, > > Neil > > -- > Neil Conway <neilconway@rogers.com> > PGP Key ID: DB3C29FC [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073