compiler warnings - Githubissues

tessus commented 4 months ago

The compiler spits out a few warnings. I suspect you want to keep the md5 code to be backwards compatible. So maybe silencing the deprecation warnings might be valid.

However, the following should be fixed in the code:

motp.c:36:48: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
   36 |     snprintf(hashbuf, sizeof(hashbuf), "%lu%s%s", counter, keybuf, pin);
      |                                                ^
motp.c:36:5: note: ‘snprintf’ output 2 or more bytes (assuming 257) into a destination of size 256
   36 |     snprintf(hashbuf, sizeof(hashbuf), "%lu%s%s", counter, keybuf, pin);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

hashbuf needs space for the termination character...

archiecobbs commented 4 months ago

Thanks. The buffer can only be too small if the length of key + pin is over 200 bytes, which is unlikely. Also nobody uses MOTP anymore. We should probably just nuke this file.

Does this patch fix the warning for you? (I don't get that warning on my system)

diff --git a/motp.c b/motp.c
index c159ba9..e7c24a2 100644
--- a/motp.c
+++ b/motp.c
@@ -29,8 +29,8 @@ void
 motp(const u_char *key, size_t keylen, const char *pin, u_long counter, int ndigits, char *buf, size_t buflen)
 {
     u_char hash[MD5_DIGEST_LENGTH];
-    char hashbuf[256];
-    char keybuf[256];
+    char keybuf[keylen * 2 + 1];
+    char hashbuf[64 + (keylen * 2) + strlen(pin)];

     printhex(keybuf, sizeof(keybuf), key, keylen, keylen * 2);
     snprintf(hashbuf, sizeof(hashbuf), "%lu%s%s", counter, keybuf, pin);

tessus commented 4 months ago

Yes, the patch fixes the warning, but I am not sure why.

You reserve space for the termination \0 character in keybuf, but not in hashbuf. So, if you write the keybuf into hashbuf and the 64 characters and the pin are used, the resulting string is one character larger than what is defined. Unless I miss something.

P.S.: Btw, I was compiling it on Fedora 40 server.

archiecobbs commented 4 months ago

The "64" is an intentional overestimate. The number of decimal digits in a 64 bit long value is always at most 19. Add a NUL byte and that would be 20. So 64 is just a brute force trick to make sure we shut up the compiler :)

archiecobbs commented 4 months ago

Fixed in 018ad3f.

tessus commented 4 months ago

The number of decimal digits in a 64 bit long value is always at most 19

Oh, I didn't check where the 64 came from. I thought it could be string of 64 characters... I ignored the fact that counter is written in the snprintf statemtnt. Now it makes perfect sense.

tessus commented 4 months ago

Oh, btw, the deprecation warnings are still there:

make  all-am
make[1]: Entering directory '/data/ext/mod-authn-otp'
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT otptool.o -MD -MP -MF .deps/otptool.Tpo -c -o otptool.o otptool.c
mv -f .deps/otptool.Tpo .deps/otptool.Po
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT hotp.o -MD -MP -MF .deps/hotp.Tpo -c -o hotp.o hotp.c
mv -f .deps/hotp.Tpo .deps/hotp.Po
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT motp.o -MD -MP -MF .deps/motp.Tpo -c -o motp.o motp.c
motp.c: In function ‘motp’:
motp.c:37:5: warning: ‘MD5’ is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
   37 |     MD5((u_char *)hashbuf, strlen(hashbuf), hash);
      |     ^~~
In file included from otptool.h:32,
                 from motp.c:20:
/usr/include/openssl/md5.h:52:38: note: declared here
   52 | OSSL_DEPRECATEDIN_3_0 unsigned char *MD5(const unsigned char *d, size_t n,
      |                                      ^~~
mv -f .deps/motp.Tpo .deps/motp.Po
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT phex.o -MD -MP -MF .deps/phex.Tpo -c -o phex.o phex.c
mv -f .deps/phex.Tpo .deps/phex.Po
gcc  -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual   -o otptool otptool.o hotp.o motp.o phex.o  -lapr-1 -lcrypto
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT otplock.o -MD -MP -MF .deps/otplock.Tpo -c -o otplock.o otplock.c
mv -f .deps/otplock.Tpo .deps/otplock.Po
gcc  -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual   -o otplock otplock.o  -lapr-1 -lcrypto
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT genotpurl.o -MD -MP -MF .deps/genotpurl.Tpo -c -o genotpurl.o genotpurl.c
mv -f .deps/genotpurl.Tpo .deps/genotpurl.Po
gcc -DHAVE_CONFIG_H -I.     -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -MT base32.o -MD -MP -MF .deps/base32.Tpo -c -o base32.o base32.c
mv -f .deps/base32.Tpo .deps/base32.Po
gcc  -O3 -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual   -o genotpurl genotpurl.o base32.o  -lapr-1 -lcrypto
if test "." != "."; then /usr/bin/cp ./mod_authn_otp.c .; fi
/usr/local/apache/bin/apxs -c -D_REENTRANT `echo -Wall -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat         -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long         -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs         -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual | sed 's/ -/ -Wc,-/g'` -l crypto mod_authn_otp.c
/usr/lib64/apr-1/build/libtool --silent --mode=compile gcc -prefer-pic -O2 -march=native  -DLINUX -D_REENTRANT -D_GNU_SOURCE  -I/usr/local/apache/include  -I/usr/include/apr-1   -I/usr/include/apr-1  -Waggregate-return -Wcast-align -Wchar-subscripts -Wcomment -Wformat -Wimplicit -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wparentheses -Wpointer-arith -Wredundant-decls -Wreturn-type -Wswitch -Wtrigraphs -Wuninitialized -Wunused -Wwrite-strings -Wshadow -Wstrict-prototypes -Wcast-qual -D_REENTRANT  -c -o mod_authn_otp.lo mod_authn_otp.c && touch mod_authn_otp.slo
mod_authn_otp.c: In function 'motp':
mod_authn_otp.c:656:5: warning: 'MD5' is deprecated: Since OpenSSL 3.0 [-Wdeprecated-declarations]
  656 |     MD5((u_char *)hashbuf, strlen(hashbuf), hash);
      |     ^~~
In file included from mod_authn_otp.c:53:
/usr/include/openssl/md5.h:52:38: note: declared here
   52 | OSSL_DEPRECATEDIN_3_0 unsigned char *MD5(const unsigned char *d, size_t n,
      |                                      ^~~
/usr/lib64/apr-1/build/libtool --silent --mode=link gcc    -o mod_authn_otp.la  -lcrypto -rpath /usr/local/apache/modules -module -avoid-version    mod_authn_otp.lo
make[1]: Leaving directory '/data/ext/mod-authn-otp'

archiecobbs commented 4 months ago

Yep .. and there are some other cleanups needed as well.

Please try 68397dd and let me know how it looks. Thanks.

tessus commented 4 months ago

Yep, no more warnings. 🎉 👍

archiecobbs commented 4 months ago

Great - thanks.

archiecobbs / mod-authn-otp

compiler warnings #49