Re: gpgme static analysis findings

On Montag, 15. Juli 2024 18:17:59 CEST Michal Hlavinka via Gnupg-devel wrote:
> Hi,
> 
> we've started to run static analysis on some system components including
> gpgme where the scanner reported a few issues:

Thanks.

> 1) in src/engine.c
> """
> "Error: OVERRUN (CWE-119):
> src/engine.c:121: cond_between: Checking ""proto > 7UL"" implies that
> ""proto"" is between 0 and 7 (inclusive) on the false branch.
> src/engine.c:125: overrun-local: Overrunning array ""engine_ops"" of 7
> 8-byte elements at element index 7 (byte offset 63) using index
> ""proto"" (which evaluates to 7).
> #  123|
> #  124|     if (engine_ops[proto] && engine_ops[proto]->get_req_version)
> #  125|->     return (*engine_ops[proto]->get_req_version) ();
> #  126|     else
> #  127|       return NULL;"
> """
> 
> 121:   if (proto > DIM (engine_ops))
> 
> this checks if 'proto' is bigger than number of elements in engine_ops
> 
> DIM is defined in util.h:
> 44: #define DIM(v) (sizeof(v)/sizeof((v)[0]))
> 
> the above condition allows for proto = DIM(engine_ops)
> which later at:
> 125:    return (*engine_ops[proto]->get_req_version) ();
> allows to access (proto+1)th element
> 
> seems that the condition should actually be proto >= DIM(engine_ops)

I agree. There's no real danger of an overrun because the enum enumerating the 
protocols only lists 7 values (0-6) and the two additional enum values 254 and 
255 which are much > DIM (engine_ops). Of course, this should still be fixed.

> 2) in src/gpgme-tool.c
> simple list assignment as used at gt_get_keylist_mode () does not
> prevent out of bound access.
> 
> """
> Error: OVERRUN (CWE-119):
> src/gpgme-tool.c:1445: assignment: Assigning: ""idx"" = ""0"".
> src/gpgme-tool.c:1449: incr: Incrementing ""idx"". The value of ""idx""
> is now 1.
> src/gpgme-tool.c:1451: incr: Incrementing ""idx"". The value of ""idx""
> is now 2.
> src/gpgme-tool.c:1453: incr: Incrementing ""idx"". The value of ""idx""
> is now 3.
> src/gpgme-tool.c:1455: incr: Incrementing ""idx"". The value of ""idx""
> is now 4.
> src/gpgme-tool.c:1457: incr: Incrementing ""idx"". The value of ""idx""
> is now 5.
> src/gpgme-tool.c:1459: incr: Incrementing ""idx"". The value of ""idx""
> is now 6.
> src/gpgme-tool.c:1461: incr: Incrementing ""idx"". The value of ""idx""
> is now 7.
> src/gpgme-tool.c:1463: incr: Incrementing ""idx"". The value of ""idx""
> is now 8.
> src/gpgme-tool.c:1464: overrun-local: Overrunning array ""modes"" of 7
> 8-byte elements at element index 8 (byte offset 71) using index
> ""idx++"" (which evaluates to 8).
> # 1462|     if (mode & GPGME_KEYLIST_MODE_FORCE_EXTERN)
> # 1463|       modes[idx++] = ""force_extern"";
> # 1464|->   modes[idx++] = NULL;
> # 1465|
> # 1466|     gt_write_status (gt, STATUS_KEYLIST_MODE, modes[0],
> modes[1], modes[2],
> """

This has been fixed. And while at it I have added support for some more keylist 
modes. https://dev.gnupg.org/rMaa15a664b3cf9bf578ba6d22c1c0c327af68b1b4

Regards,
Ingo

_______________________________________________
Gnupg-devel mailing list
[email protected]
https://lists.gnupg.org/mailman/listinfo/gnupg-devel

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTbjgIOMowwlCBgvyGxb1mVFkdKugUCZpWLlAAKCRCxb1mVFkdK
uprKAQDzYTJYw2ZniNiopf+jYDZF7u1H5BdY4XGITliSPUXqhQD/broPTwzSuXR8
Rcz0LaByuwBmcStrnaOQ83RnPryOsg0=
=3EX5
-----END PGP SIGNATURE-----