Hacker News Reader

Comment by michaelmior | Hacker News Reader

It's a little scary for me that the source of available package is a wiki page which is editable by anyone with a GitHub account.

View on HN

Not only that, but there appear to be potential buffer overflows which involve the data pulled from the wiki page[1]. I'm not sure why asprintf wasn't used here (and in other places). The partial re-implementation of libc[2, 3] is also somewhat frightening. As are potential unchecked integer overflows when calculating a size to pass to malloc[4].

C is hard.

[1] https://github.com/clibs/clib/blob/master/src/clib-install.c...

[2] https://github.com/clibs/clib/blob/master/deps/fs/fs.c

[3] https://github.com/clibs/clib/blob/master/deps/str-copy/str-...

[4] https://github.com/clibs/clib/blob/master/deps/http-get/http...

Oh dear... I'm just sight-linting so this might be wrong, but I'm certain the function executable in src/clib-install.c [1] can be easily overflown by providing a maliciously long package name or version --neither of which are validated at all [2] despite coming straight from the wiki page, as you point out. The function allocates 256-byte buffers to store a url and file name, respectively, but blindly fills them up using sprintf with non-validated user-supplied data, and without checking the resulting number of characters written, eg:

  char *url = malloc(256);
  if(NULL == file) goto e1;
  sprintf(file,"%s-%s.tar.gz",pkg->name,pkg->version);

C is unforgiving indeed.

Also what's up with all those (0 == count) yoda conditionals all over? Is that becoming a thing again?

[1] https://github.com/clibs/clib/blob/master/src/clib-install.c

[2] As far as I can tell, the package info from the wiki is dealt with by functions in deps/clib-package.c that, despite promising names like json_object_get_string_safe, do not ever validate the provided name or version string:

https://github.com/clibs/clib/blob/master/deps/clib-package/...

Reply on news.ycombinator.com