Re: libc: locale/rune.c input validation
Hi Ingo,
On Fri, Dec 04, 2015 at 12:27:39PM +0100, Ingo Schwarze wrote:
> uint32_t should be preferred because that's the POSIX type, while
> u_int32_t is not standardized. If you are working on the file
> anyway, i'd recommend to unify all uses to uint32_t.
done.
> __LP64__ that is overly specific and not standardized. Something
> like this maybe:
>
> #if SIZE_MAX <= UINT32_MAX
That sounds nice and gives a better clue why that #if is in place.
I've applied it.
> > I still cast frl_variable_len to u_int32_t for ntohl, but will cast it
> > back to int32_t for a "less than 0" check. This should properly handle
> > error cases when it's been negative to begin with.
>
> True. Slightly ugly, but i don't see a better way either.
Looking at UINT32_MAX usage now, I just test for var_len > INT32_MAX.
Tobias
Index: locale/rune.c
===
RCS file: /cvs/src/lib/libc/locale/rune.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 rune.c
--- locale/rune.c 25 May 2014 17:47:04 - 1.4
+++ locale/rune.c 4 Dec 2015 20:48:52 -
@@ -59,23 +59,31 @@
* SUCH DAMAGE.
*/
+#include
+#include
#include
+#include
+#include
#include
-#include
#include
-#include
+#include
#include
-#include
-#include
#include "rune.h"
#include "rune_local.h"
-static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *,
FILE *);
+#define SAFE_ADD(x, y) \
+do { \
+ if ((x) > SIZE_MAX - (y)) \
+ return NULL;\
+ (x) += (y); \
+} while (0);
+
+static int readrange(_RuneLocale *, _RuneRange *, uint32_t, void *, FILE *);
static void _freeentry(_RuneRange *);
static void _wctype_init(_RuneLocale *rl);
static int
-readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
+readrange(_RuneLocale *rl, _RuneRange *rr, uint32_t nranges, void *lastp,
FILE *fp)
{
uint32_t i;
@@ -84,7 +92,7 @@ readrange(_RuneLocale *rl, _RuneRange *r
re = (_RuneEntry *)rl->rl_variable;
- rr->rr_nranges = ntohl(frr->frr_nranges);
+ rr->rr_nranges = nranges;
if (rr->rr_nranges == 0) {
rr->rr_rune_ranges = NULL;
return 0;
@@ -92,16 +100,16 @@ readrange(_RuneLocale *rl, _RuneRange *r
rr->rr_rune_ranges = re;
for (i = 0; i < rr->rr_nranges; i++) {
+ if ((void *)re >= lastp)
+ return -1;
+
if (fread(, sizeof(fre), 1, fp) != 1)
return -1;
- re->re_min = ntohl((u_int32_t)fre.fre_min);
- re->re_max = ntohl((u_int32_t)fre.fre_max);
- re->re_map = ntohl((u_int32_t)fre.fre_map);
+ re->re_min = ntohl((uint32_t)fre.fre_min);
+ re->re_max = ntohl((uint32_t)fre.fre_max);
+ re->re_map = ntohl((uint32_t)fre.fre_map);
re++;
-
- if ((void *)re > lastp)
- return -1;
}
rl->rl_variable = re;
return 0;
@@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
continue;
}
+ if (re[i].re_max < re[i].re_min)
+ goto fail;
+
l = re[i].re_max - re[i].re_min + 1;
re[i].re_rune_types = calloc(l, sizeof(_RuneType));
if (!re[i].re_rune_types) {
@@ -151,17 +162,20 @@ fail2:
}
/* XXX: temporary implementation */
-static void
+static int
find_codeset(_RuneLocale *rl)
{
char *top, *codeset, *tail, *ep;
+ if (rl->rl_variable == NULL)
+ return 0;
+
/* end of rl_variable region */
ep = (char *)rl->rl_variable;
ep += rl->rl_variable_len;
rl->rl_codeset = NULL;
if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
- return;
+ return 0;
tail = strpbrk(top, " \t");
codeset = top + sizeof(_RUNE_CODESET) - 1;
if (tail) {
@@ -173,6 +187,7 @@ find_codeset(_RuneLocale *rl)
*top = '\0';
rl->rl_codeset = strdup(codeset);
}
+ return (rl->rl_codeset == NULL);
}
void
@@ -183,8 +198,7 @@ _freeentry(_RuneRange *rr)
re = rr->rr_rune_ranges;
for (i = 0; i < rr->rr_nranges; i++) {
- if (re[i].re_rune_types)
- free(re[i].re_rune_types);
+ free(re[i].re_rune_types);
re[i].re_rune_types = NULL;
}
}
@@ -209,6 +223,7 @@ _Read_RuneMagi(FILE *fp)
_RuneLocale *rl;
struct stat sb;
int x;
+ uint32_t runetype_nranges, maplower_nranges, mapupper_nranges, var_len;
if (fstat(fileno(fp), ) < 0)
return NULL;
@@ -225,10 +240,26 @@ _Read_RuneMagi(FILE *fp)
if (memcmp(frl.frl_magic, _RUNE_MAGIC_1,
Re: libc: locale/rune.c input validation
Thanks Ingo for your extensive review! It contains lots of valuable
input for me.
I have applied all your recommendations, they make a lot of sense.
> I would suggest to use uint32_t.
Just while applying this, I noticed that the file has a mix of the
types u_int32_t and uint32_t. I took u_int32_t for now because it was
in there more often than uint32_t. What do you think?
> > +#ifndef __LP64__
> > + if (runetype_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> > + maplower_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
> > + mapupper_nranges > SIZE_MAX / sizeof(_RuneEntry))
> > + return NULL;
> > +#endif
>
> I dislike using #ifdef, it seems error-prone to me. The checks
> are correct in any case. Sure, if size_t is much larger than uint32_t,
> these checks can never trigger. But why do micro-optimization here?
> Why not just delete the #ifndef/#endif?
If I skip this __LP64__ check, it triggers a gcc warning on amd64,
because the compiler tells me that the test is useless. So it's a
cosmetical thing. It's removed for now.
I still cast frl_variable_len to u_int32_t for ntohl, but will cast it
back to int32_t for a "less than 0" check. This should properly handle
error cases when it's been negative to begin with.
Tobias
Index: lib/libc/locale/rune.c
===
RCS file: /cvs/src/lib/libc/locale/rune.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 rune.c
--- locale/rune.c 25 May 2014 17:47:04 - 1.4
+++ locale/rune.c 3 Dec 2015 21:55:54 -
@@ -59,32 +59,40 @@
* SUCH DAMAGE.
*/
+#include
+#include
#include
+#include
+#include
#include
-#include
#include
-#include
+#include
#include
-#include
-#include
#include "rune.h"
#include "rune_local.h"
-static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *,
FILE *);
+#define SAFE_ADD(x, y) \
+do { \
+ if ((x) > SIZE_MAX - (y)) \
+ return NULL;\
+ (x) += (y); \
+} while (0);
+
+static int readrange(_RuneLocale *, _RuneRange *, u_int32_t, void *, FILE *);
static void _freeentry(_RuneRange *);
static void _wctype_init(_RuneLocale *rl);
static int
-readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
+readrange(_RuneLocale *rl, _RuneRange *rr, u_int32_t nranges, void *lastp,
FILE *fp)
{
- uint32_t i;
+ u_int32_t i;
_RuneEntry *re;
_FileRuneEntry fre;
re = (_RuneEntry *)rl->rl_variable;
- rr->rr_nranges = ntohl(frr->frr_nranges);
+ rr->rr_nranges = nranges;
if (rr->rr_nranges == 0) {
rr->rr_rune_ranges = NULL;
return 0;
@@ -92,6 +100,9 @@ readrange(_RuneLocale *rl, _RuneRange *r
rr->rr_rune_ranges = re;
for (i = 0; i < rr->rr_nranges; i++) {
+ if ((void *)re >= lastp)
+ return -1;
+
if (fread(, sizeof(fre), 1, fp) != 1)
return -1;
@@ -99,9 +110,6 @@ readrange(_RuneLocale *rl, _RuneRange *r
re->re_max = ntohl((u_int32_t)fre.fre_max);
re->re_map = ntohl((u_int32_t)fre.fre_map);
re++;
-
- if ((void *)re > lastp)
- return -1;
}
rl->rl_variable = re;
return 0;
@@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
continue;
}
+ if (re[i].re_max < re[i].re_min)
+ goto fail;
+
l = re[i].re_max - re[i].re_min + 1;
re[i].re_rune_types = calloc(l, sizeof(_RuneType));
if (!re[i].re_rune_types) {
@@ -151,17 +162,20 @@ fail2:
}
/* XXX: temporary implementation */
-static void
+static int
find_codeset(_RuneLocale *rl)
{
char *top, *codeset, *tail, *ep;
+ if (rl->rl_variable == NULL)
+ return 0;
+
/* end of rl_variable region */
ep = (char *)rl->rl_variable;
ep += rl->rl_variable_len;
rl->rl_codeset = NULL;
if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
- return;
+ return 0;
tail = strpbrk(top, " \t");
codeset = top + sizeof(_RUNE_CODESET) - 1;
if (tail) {
@@ -173,18 +187,18 @@ find_codeset(_RuneLocale *rl)
*top = '\0';
rl->rl_codeset = strdup(codeset);
}
+ return (rl->rl_codeset == NULL);
}
void
_freeentry(_RuneRange *rr)
{
_RuneEntry *re;
- uint32_t i;
+ u_int32_t i;
re = rr->rr_rune_ranges;
for (i = 0; i < rr->rr_nranges; i++) {
- if (re[i].re_rune_types)
- free(re[i].re_rune_types);
+ free(re[i].re_rune_types);
re[i].re_rune_types = NULL;
}
}
@@ -209,6 +223,7 @@
libc: locale/rune.c input validation
Hi,
this patch adds a lot of input validation to libc/locale/rune.c.
The kind of validations are borrowed from my nls changes some
weeks ago.
I've contacted stsp@ about this. I think it's ready to get more
review from tech@. Let me know what you think!
Tobias
Index: rune.c
===
RCS file: /cvs/src/lib/libc/locale/rune.c,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 rune.c
--- rune.c 25 May 2014 17:47:04 - 1.4
+++ rune.c 30 Oct 2015 16:13:01 -
@@ -59,23 +59,31 @@
* SUCH DAMAGE.
*/
+#include
+#include
#include
+#include
+#include
#include
-#include
#include
-#include
+#include
#include
-#include
-#include
#include "rune.h"
#include "rune_local.h"
-static int readrange(_RuneLocale *, _RuneRange *, _FileRuneRange *, void *,
FILE *);
+#define SAFE_ADD(x, y) \
+do { \
+ if ((x) > SIZE_MAX - (y)) \
+ return NULL;\
+ (x) += (y); \
+} while (0);
+
+static int readrange(_RuneLocale *, _RuneRange *, rune_t, void *, FILE *);
static void _freeentry(_RuneRange *);
static void _wctype_init(_RuneLocale *rl);
static int
-readrange(_RuneLocale *rl, _RuneRange *rr, _FileRuneRange *frr, void *lastp,
+readrange(_RuneLocale *rl, _RuneRange *rr, rune_t nranges, void *lastp,
FILE *fp)
{
uint32_t i;
@@ -84,7 +92,7 @@ readrange(_RuneLocale *rl, _RuneRange *r
re = (_RuneEntry *)rl->rl_variable;
- rr->rr_nranges = ntohl(frr->frr_nranges);
+ rr->rr_nranges = nranges;
if (rr->rr_nranges == 0) {
rr->rr_rune_ranges = NULL;
return 0;
@@ -92,6 +100,9 @@ readrange(_RuneLocale *rl, _RuneRange *r
rr->rr_rune_ranges = re;
for (i = 0; i < rr->rr_nranges; i++) {
+ if ((void *)re >= lastp)
+ return -1;
+
if (fread(, sizeof(fre), 1, fp) != 1)
return -1;
@@ -99,9 +110,6 @@ readrange(_RuneLocale *rl, _RuneRange *r
re->re_max = ntohl((u_int32_t)fre.fre_max);
re->re_map = ntohl((u_int32_t)fre.fre_map);
re++;
-
- if ((void *)re > lastp)
- return -1;
}
rl->rl_variable = re;
return 0;
@@ -121,6 +129,9 @@ readentry(_RuneRange *rr, FILE *fp)
continue;
}
+ if (re[i].re_max < re[i].re_min)
+ goto fail;
+
l = re[i].re_max - re[i].re_min + 1;
re[i].re_rune_types = calloc(l, sizeof(_RuneType));
if (!re[i].re_rune_types) {
@@ -151,17 +162,20 @@ fail2:
}
/* XXX: temporary implementation */
-static void
+static int
find_codeset(_RuneLocale *rl)
{
char *top, *codeset, *tail, *ep;
+ if (rl->rl_variable == NULL)
+ return 0;
+
/* end of rl_variable region */
ep = (char *)rl->rl_variable;
ep += rl->rl_variable_len;
rl->rl_codeset = NULL;
if (!(top = strstr(rl->rl_variable, _RUNE_CODESET)))
- return;
+ return 0;
tail = strpbrk(top, " \t");
codeset = top + sizeof(_RUNE_CODESET) - 1;
if (tail) {
@@ -173,6 +187,7 @@ find_codeset(_RuneLocale *rl)
*top = '\0';
rl->rl_codeset = strdup(codeset);
}
+ return (rl->rl_codeset == NULL);
}
void
@@ -183,8 +198,7 @@ _freeentry(_RuneRange *rr)
re = rr->rr_rune_ranges;
for (i = 0; i < rr->rr_nranges; i++) {
- if (re[i].re_rune_types)
- free(re[i].re_rune_types);
+ free(re[i].re_rune_types);
re[i].re_rune_types = NULL;
}
}
@@ -209,6 +223,7 @@ _Read_RuneMagi(FILE *fp)
_RuneLocale *rl;
struct stat sb;
int x;
+ rune_t runetype_nranges, maplower_nranges, mapupper_nranges;
if (fstat(fileno(fp), ) < 0)
return NULL;
@@ -225,10 +240,24 @@ _Read_RuneMagi(FILE *fp)
if (memcmp(frl.frl_magic, _RUNE_MAGIC_1, sizeof(frl.frl_magic)))
return NULL;
- hostdatalen = sizeof(*rl) + ntohl((u_int32_t)frl.frl_variable_len) +
- ntohl(frl.frl_runetype_ext.frr_nranges) * sizeof(_RuneEntry) +
- ntohl(frl.frl_maplower_ext.frr_nranges) * sizeof(_RuneEntry) +
- ntohl(frl.frl_mapupper_ext.frr_nranges) * sizeof(_RuneEntry);
+ /* XXX assumes rune_t = u_int32_t */
+ runetype_nranges = ntohl(frl.frl_runetype_ext.frr_nranges);
+ maplower_nranges = ntohl(frl.frl_maplower_ext.frr_nranges);
+ mapupper_nranges = ntohl(frl.frl_mapupper_ext.frr_nranges);
+
+#ifndef __LP64__
+ if (runetype_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
+ maplower_nranges > SIZE_MAX / sizeof(_RuneEntry) ||
+ mapupper_nranges >
