[openssl.org #2910] OPENSSL_cleanse called with wrong size

[Kees Cook via RT]

http://www.viva64.com/en/b/0178/

OPENSSL_cleanse is being called with pointer size instead of the buffer size in some places.
For example crypto/des/des.c:

void doencryption(void)
...
static unsigned char *buf=NULL,*obuf=NULL;
...
OPENSSL_cleanse(buf,sizeof(buf));
OPENSSL_cleanse(obuf,sizeof(obuf));

This is leaving memory uncleared.

[Mansour Moufid]

Post by Kees Cook via RT
http://www.viva64.com/en/b/0178/
OPENSSL_cleanse is being called with pointer size instead of the buffer
size in some places.
void doencryption(void)
...
static unsigned char *buf=NULL,*obuf=NULL;
...
OPENSSL_cleanse(buf,sizeof(buf));
OPENSSL_cleanse(obuf,sizeof(obuf));
This is leaving memory uncleared.
--

I wrote a semantic patch (for Coccinelle) which detects and fixes this
kind of bug, see attached `buggy-OPENSSL_cleanse.cocci'. For example,
here is its output for the file crypto/des/des.c:

$ spatch --sp-file buggy-OPENSSL_cleanse.cocci \
openssl-1.0.1c/crypto/des/des.c
...
@@ -666,8 +666,8 @@ void doencryption(void)
if (l) fclose(CKSUM_OUT);
}
problems:
- OPENSSL_cleanse(buf,sizeof(buf));
- OPENSSL_cleanse(obuf,sizeof(obuf));
+ OPENSSL_cleanse(buf, BUFSIZE + 8);
+ OPENSSL_cleanse(obuf, BUFSIZE + 8);
OPENSSL_cleanse(&ks,sizeof(ks));
OPENSSL_cleanse(&ks2,sizeof(ks2));
OPENSSL_cleanse(iv,sizeof(iv));

It seems that one is not the only instance. Try:

$ spatch --sp-file buggy-OPENSSL_cleanse.cocci --dir openssl-1.0.1c \
| tee openssl-1.0.1c-buggy-OPENSSL_cleanse.patch

Attached is that patch, which I cleaned up because of some false
positives and other unwanted stuff. I'm not familiar enough with the
OpenSSL code to judge which parts of it are relevant.

Mansour

[Andy Polyakov]

Post by Mansour Moufid
$ spatch --sp-file buggy-OPENSSL_cleanse.cocci --dir openssl-1.0.1c \
| tee openssl-1.0.1c-buggy-OPENSSL_cleanse.patch
Attached is that patch, which I cleaned up because of some false
positives and other unwanted stuff. I'm not familiar enough with the
OpenSSL code to judge which parts of it are relevant.

With exception for [obsoleted and never compiled] des.c none. In sense
that original code is not wrong, while replacement is.
______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Andy Polyakov via RT]

Post by Kees Cook via RT
OPENSSL_cleanse is being called with pointer size instead of the buffer size in some places.
void doencryption(void)
...
static unsigned char *buf=NULL,*obuf=NULL;
...
OPENSSL_cleanse(buf,sizeof(buf));
OPENSSL_cleanse(obuf,sizeof(obuf));
This is leaving memory uncleared.

Note that file in question is never compiled. The file in question can
as well be omitted from source distribution...


______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org

[Rich Salz via RT]

Per Andy, removing des.c and its targets from crypto/des/Makefile.

There is more old code to remove...
--
Rich Salz, OpenSSL dev team; ***@openssl.org

______________________________________________________________________
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-***@openssl.org
Automated List Manager ***@openssl.org