Secure coding practices – would you expect the RFC to follow them?

We recently did some work finding inherited vulnerabilities in SNMP supporting devices – mainly embedded/hardware.

SNMP like many other protocols is defined in several RFCs, starting with the basic RFC that describes the protocol structure and goes up to RFC 3414 which describes how authentication and encryption (referred to as privacy by the SNMP spec) are done.

The RFC describes a few algorithms, such as the one for key localization, in great detail – i.e. providing a C source code:


void password_to_key_md5(
u_char *password, /* IN */
u_int passwordlen, /* IN */
u_char *engineID, /* IN - pointer to snmpEngineID */
u_int engineLength,/* IN - length of snmpEngineID */
u_char *key) /* OUT - pointer to caller 16-octet buffer */
{
MD5_CTX MD;
u_char *cp, password_buf[64];
u_long password_index = 0;
u_long count = 0, i;

MD5Init (&MD); /* initialize MD5 */

/**********************************************/
/* Use while loop until we've done 1 Megabyte */
/**********************************************/
while (count < 1048576) {
cp = password_buf;
for (i = 0; i < 64; i++) {
/*************************************************/
/* Take the next octet of the password, wrapping */
/* to the beginning of the password as necessary.*/
/*************************************************/
*cp++ = password[password_index++ % passwordlen];
}
MD5Update (&MD, password_buf, 64);
count += 64;
}
MD5Final (key, &MD); /* tell MD5 we're done */

/*****************************************************/
/* Now localize the key with the engineID and pass */
/* through MD5 to produce final key */
/* May want to ensure that engineLength <= 32, */
/* otherwise need to use a buffer larger than 64 */
/*****************************************************/
memcpy(password_buf, key, 16);
memcpy(password_buf+16, engineID, engineLength);
memcpy(password_buf+16+engineLength, key, 16);

MD5Init(&MD);
MD5Update(&MD, password_buf, 32+engineLength);
MD5Final(key, &MD);
return;
}

People reading this RFC would jump with joy and just copy paste the above code into their own code and continue on their work of getting SNMP to work on their hardware.

Little will they know that the RFC team has made notice that:

May want to ensure that engineLength < = 32, otherwise need to use a buffer larger than 64

“Ohh pff, who needs to ensure anything on my robust hardware – what is the worst that can happen, a server crash :D

Actually, the worst that can happen is a neat buffer overflow, as no one guarantees that the engineID is limited to 32 bytes, in reality the length is not limited by the transport layer (the ASN.1) but rather only by the RFC specification, which again not everyone checks or conforms to it by 100%.

I would expect the RFC editors/creators to place sample code that is secure. Something like so would have sufficed to prevent the code from being easily exploited:
memcpy(password_buf, key, 16);
memcpy(password_buf+16, engineID, engineLength < 32 ? engineLength : 32);
memcpy(password_buf+16+(engineLength < 32 ? engineLength : 32), key, 16);

Especially since Google searching the above example proved that quite a few people were too lazy to not only fix the security issue but too lazy to remove the embarrassing comment.

Share