On Tue, Jan 21, 2020 at 05:22:17PM -0500, Ted Unangst wrote:
> Ted Unangst wrote:
> > MarcusMüller wrote:
> > > I've just stumbled across a malfunction in signify: It cannot handle
> > > file names that contain a `)` character, when checking a list of hashes
> > > generated by `sha256` command line utilities (`sha256sum --tags` on
> > > Linux).
> >
> > This fix is unfortunately rather complicated for the problem. Files with )
> > are
> > not used within openbsd, for instance. It may possible to simplify it a
> > bit?
>
> Not much simpler, but maybe this is easier to follow. Keeps the hair in a
> separate function.
>
>
> Index: signify.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/signify/signify.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 signify.c
> --- signify.c 22 Dec 2019 06:37:25 -0000 1.134
> +++ signify.c 21 Jan 2020 22:20:55 -0000
> @@ -651,6 +651,37 @@ verifychecksum(struct checksum *c, int q
> }
>
> static void
> +scanchecksum(const char *input, struct checksum *c)
> +{
> + char *p, *algo, *file, *hash;
> + char line[2 * PATH_MAX];
> +
Hi,
Maybe the line buffer should be: PATH_MAX + HASHBUFSIZE + 32 + 6 (for ") = "
and " (") ?
Struct checksum hash:
#define HASHBUFSIZE 224
struct checksum {
char file[PATH_MAX];
char hash[HASHBUFSIZE];
char algo[32];
};
PATH_MAX can be >= 256 bytes.
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html
> + if (strlcpy(line, input, sizeof(line)) >= sizeof(line))
> + goto fail;
> +
There is some white-space before the goto;
> + /* algo (filename) = hash */
> + algo = line;
> + p = strchr(algo, ' ');
> + if (p == NULL || strncmp(p, " (", 2) != 0)
> + goto fail;
> + *p = 0;
Nitpicking: maybe *p = '\0' is a bit nicer instead of *p = 0.
> + file = p + 2;
> + p = strrchr(file, ')');
> + if (p == NULL || strncmp(p, ") = ", 4) != 0)
> + goto fail;
> + *p = 0;
> + hash = p + 4;
> +
> + if (strlcpy(c->algo, algo, sizeof(c->algo)) >= sizeof(c->algo) ||
> + strlcpy(c->file, file, sizeof(c->file)) >= sizeof(c->file) ||
> + strlcpy(c->hash, hash, sizeof(c->hash)) >= sizeof(c->hash))
> + goto fail;
> + return;
> +fail:
> + errx(1, "unable to parse checksum line %s", input);
> +}
> +
> +static void
> verifychecksums(char *msg, int argc, char **argv, int quiet)
> {
> struct ohash_info info = { 0, NULL, ecalloc, efree, NULL };
> @@ -658,7 +689,7 @@ verifychecksums(char *msg, int argc, cha
> struct checksum c;
> char *e, *line, *endline;
> int hasfailed = 0;
> - int i, rv;
> + int i;
> unsigned int slot;
>
> ohash_init(&myh, 6, &info);
> @@ -675,13 +706,8 @@ verifychecksums(char *msg, int argc, cha
> while (line && *line) {
> if ((endline = strchr(line, '\n')))
> *endline++ = '\0';
> -#if PATH_MAX < 1024 || HASHBUFSIZE < 224
> -#error sizes are wrong
> -#endif
> - rv = sscanf(line, "%31s (%1023[^)]) = %223s",
> - c.algo, c.file, c.hash);
> - if (rv != 3)
> - errx(1, "unable to parse checksum line %s", line);
> +
> + scanchecksum(line, &c);
> line = endline;
> if (argc) {
> slot = ohash_qlookup(&myh, c.file);
>
Briefly tested and it looks otherwise good to me.
Thanks,
--
Kind regards,
Hiltjo
