As reported in: https://bugzilla.redhat.com/show_bug.cgi?id=1251018 The first patch is a cleanup and the others add support for those attributes.
Created attachment 125597 [details] [review] uri: Remove whitespace early when parsing For every path/query component, p11_kit_uri_parse() allocates a small buffer to strip whitespace out. This patch removes any whitespace in the URI at the entry of the function to simplify the code. Note that RFC 7512 actually suggests to ignore whitespace at the extracting phase rather than the parsing phase.
Created attachment 125598 [details] [review] uri: Support CK_SLOT_INFO path attributes Accept 'slot-description' and 'slot-manifacturer' path attributes defined in RFC 7512.
Created attachment 125599 [details] [review] uri: Support 'slot-id' attribute Accept 'slot-id' path attribute defined in RFC 7512.
Created attachment 125600 [details] [review] uri: Support 'module-name' and 'module-path' Accept 'module-name' and 'module-path' query attributes defined in RFC 7512.
Comment on attachment 125597 [details] [review] uri: Remove whitespace early when parsing Review of attachment 125597 [details] [review]: ----------------------------------------------------------------- Mostly looks good. I assume this is to make implementing the following patches easier. This patch needs tests covering each valid input, each path, and each failure mode. ::: p11-kit/uri.c @@ +1046,5 @@ > > if (dot == start) > return P11_KIT_URI_BAD_VERSION; > + endptr = NULL; > + val = strtol (start, &endptr, 10); Can't this cause access to memory after |end| in the case where the number is at the end of the string. It could fail when |end| is the back of a memory page, or |end| happens to be trailed by numbers until the back of a memory page.
Comment on attachment 125598 [details] [review] uri: Support CK_SLOT_INFO path attributes Review of attachment 125598 [details] [review]: ----------------------------------------------------------------- Looks good to me.
Comment on attachment 125599 [details] [review] uri: Support 'slot-id' attribute Review of attachment 125599 [details] [review]: ----------------------------------------------------------------- This adds two warnings: /data/src/p11-kit/x86_64/../p11-kit/uri.c:1289:17: warning: unused variable ‘module_path’ [-Wunused-variable] unsigned char *module_path; ^ /data/src/p11-kit/x86_64/../p11-kit/uri.c:1288:17: warning: unused variable ‘module_name’ [-Wunused-variable] ::: p11-kit/uri.c @@ +1230,5 @@ > + if (memcmp ("slot-id", name_start, name_end - name_start) == 0) { > + char *endptr; > + unsigned long slot_id; > + endptr = NULL; > + slot_id = strtoul (start, &endptr, 10); Seems this can result in accessing memory out of bounds, if the number runs all the way until |end|.
Comment on attachment 125600 [details] [review] uri: Support 'module-name' and 'module-path' Review of attachment 125600 [details] [review]: ----------------------------------------------------------------- Looks good to me.
In general I like this. I do have one more question though, does this affect the rest of p11-kit at all. What about iter.c, enumerate.c or list.c. Will you be updating those as a follow up to take these changes into account?
(In reply to Stef Walter from comment #9) > In general I like this. I do have one more question though, does this affect > the rest of p11-kit at all. What about iter.c, enumerate.c or list.c. Will > you be updating those as a follow up to take these changes into account? Thank you for the review. I completely missed the use of P11KitUri in iter.c. I will submit patches for this here as well.
Comment on attachment 125600 [details] [review] uri: Support 'module-name' and 'module-path' After pondering over this, I doubt that "module-name" and "module-path" are really useful in the context of p11-kit. They are query attribute (not path attribute) which doesn't affect the matching behaviour but modify the default settings for accessing module. As far as I understand, that task should be done by p11-kit configuration.
Created attachment 125704 [details] [review] uri: Remove whitespace early when parsing For every path/query component, p11_kit_uri_parse() allocates a small buffer to strip whitespace out. This patch removes any whitespace in the URI at the entry of the function to simplify the code. Note that RFC 7512 actually suggests to ignore whitespace at the extracting phase rather than the parsing phase. -- Restored atoin() and replaced the strtol() use with it.
Created attachment 125705 [details] [review] uri: Support slot info path attributes Accept 'slot-description' and 'slot-manifacturer' path attributes defined in RFC 7512. -- Just a summary line change, to match the subsequent patches
Created attachment 125706 [details] [review] uri: Support 'slot-id' path attribute Accept 'slot-id' path attribute defined in RFC 7512. -- Use atoin() instead of strtoul().
Created attachment 125707 [details] [review] iter: Utilize slot info URI path attributes
Created attachment 125708 [details] [review] iter: Utilize 'slot-id' URI path attribute
Nice. Merged.
Created attachment 126649 [details] [review] uri: Fix buffer overflow in memcmp() The commit 63644dc introduced several memcmp() calls without checking the length of the first argument. -- I apologize that the first patch was immature and requires this fix.
Comment on attachment 126649 [details] [review] uri: Fix buffer overflow in memcmp() Review of attachment 126649 [details] [review]: ----------------------------------------------------------------- Could you add a test that verifies this fix? A test of parsing a URL that exposed this bug?
(In reply to Stef Walter from comment #19) > Comment on attachment 126649 [details] [review] [review] > uri: Fix buffer overflow in memcmp() > > Review of attachment 126649 [details] [review] [review]: > ----------------------------------------------------------------- > > Could you add a test that verifies this fix? A test of parsing a URL that > exposed this bug? It was exposed by ASAN, using the existing test-uri program.
To reproduce, you could try: NOCONFIGURE=1 ./autogen.sh mkdir build cd build ../configure CFLAGS="-fsanitize=address -g -fno-common -U_FORTIFY_SOURCE" CXXFLAGS="-fsanitize=address -g -fno-common -U_FORTIFY_SOURCE" LDFLAGS="-fsanitize=address -g -fno-common -U_FORTIFY_SOURCE" LIBS="-ldl -lpthread" make ./test-uri cf. https://mail.gnome.org/archives/desktop-devel-list/2016-September/msg00037.html
Created attachment 126743 [details] [review] uri: Port to PKCS#11 GNU calling convention -- One more thing I noticed while building GnuTLS with git master, sorry.
Reopening so that the additional two fixes are not forgotten.
The remaining patches were merged into git: https://github.com/p11-glue/p11-kit/pull/5
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.