User Name:


Forgot your password?
Vote for Us!
auth_update crash
Dec 23, 2017, 10:15 pm
By Remcon
Dec 18, 2017, 7:21 pm
By Remcon
parse description bug
Dec 15, 2017, 10:08 pm
By Remcon
Couple bugs
Dec 12, 2017, 5:42 pm
By Remcon
Bug in disarm( )
Nov 12, 2017, 6:54 pm
By GatewaySysop
LoP 1.46
Author: Remcon
Submitted by: Remcon
LOP 1.45
Author: Remcon
Submitted by: Remcon
LOP Heroes Edition
Author: Vladaar
Submitted by: Vladaar
Heroes sound extras
Author: Vladaar
Submitted by: Vladaar
6Dragons 4.3
Author: Vladaar
Submitted by: Vladaar
Users Online
CommonCrawl, Bing, bastian

Members: 1
Guests: 11
Newest Member
Today's Birthdays
There are no member birthdays today.
Related Links
» SmaugMuds.org » Bugfix Lists » SmaugFUSS Bugfix List » [Bug] MD5 Password code has a...
Forum Rules | Mark all | Recent Posts

[Bug] MD5 Password code has a serious memory flaw
< Newer Topic :: Older Topic >

Pages:<< prev 1 next >>
Post is unread #1 Mar 16, 2005, 7:54 pm   Last edited Feb 4, 2006, 4:33 pm by Samson
Go to the top of the page
Go to the bottom of the page

Black Hand
JoinedJan 1, 2002

Bug: MD5 Password code has a serious memory flaw.
Danger: Critical - Will invalidate passwords under random circumstances.
Found by: Gatewaysysop
Fixed by: Samson/Conner

This fix now only necessary if your copy of SMAUG uses MD5 passwords

comm.c, smaug_crypt

   md5_state_t state;
   md5_byte_t digest[16];
   static char passwd[16];
   unsigned int x;

   md5_init( &state );
   md5_append( &state, ( const md5_byte_t * )pwd, strlen( pwd ) );
   md5_finish( &state, digest );

   strncpy( passwd, ( const char * )digest, 16 );

Change to:
   md5_state_t state;
   md5_byte_t digest[17];
   static char passwd[17];
   unsigned int x;

   md5_init( &state );
   md5_append( &state, ( const md5_byte_t * )pwd, strlen( pwd ) );
   md5_finish( &state, digest );

   strncpy( passwd, ( const char * )digest, 16 );
   passwd[16] = '\0';

While this may seem innocuous, there is actually a nasty memory problem lurking here. For those who may know, it should be fairly obvious. For those who don't, strncpy does not NULL terminate a string if the results of it's operation will consume the size specified. In this case, 16 bytes. All 16 bytes are generally occupied by the md5 algorithm, so the string is never properly terminated. But in order to safely terminate at the 16th byte, the digest and passwd variables need to be declared to hold 17. This fix will not cause your already saved passwords to be invalidated.

A bit of explanation. Gatewaysysop noticed that his password was getting corrupted with junk data that should not have been there. It was most noticeable when using the formpass command to test with, but it was also clobbering the password pointer on his character data as well. He does his development work on Cygwin, with a modified base. This bug apparently did not seem to phase my Linux install and everything was working fine. However in the course of our investigating this issue, it was found to affect the FUSS packages without being modified. The circumstances which brought forth the bug were rather strange. Apparently somehow when objects get grouped, they do weirdness in memory, because only when displaying grouped objects, like "A sharp knife (3)" will this bug manifest.

It struck me that in AFKMud I use the strlcpy, note the L, not N, and there are no issues with the code as is. I was not able to reproduce any of the known conditions that could cause this. I realized that it must have been due to non-terminated strings and decided to play a hunch and see what happened. Terminating the string stopped the problem. It is not known for sure if this bug would have affected other platforms, or even just other versions of GCC. No sense in chancing it.

Chalk one up to those rare conditions or something.

Update: Conner@Land of Legends noted that the original fix for this did indeed invalidate passwords if you already had the bugged system installed. So this fix was modified to correct that problem. It was also discovered that the admin account in the distro package had been affected by this as well and should now be corrected.
Pages:<< prev 1 next >>