[openssl-dev] [openssl.org #4076] PROBLEM： there exists a wrong return value of function int_rsa_verify()

[Matt Caswell via RT]

On Thu Oct 08 08:55:45 2015, rucsoftsec at 163.com wrote:
> Bug Description:
> Function int_rsa_verify() defined in file crypto/rsa/rsa_sign.c would
> return 1 if a signature is valid, and 0 otherwise. The variable 'ret'
> keeps the return value, and it may be assigned to 1 if the condition
> in line 216 is satisfied. The signature is regarded as invalid if the
> conditions in line 241 are evaluated to be true, and the error message
> is dumped (in line 242) and the verify process is ended (in line 243).
> However, as variable 'ret' may keep value 1, this function will return
> 1 (in line 290) even if the signature is invalid, which will confuse
> the caller function whether the signature is really valid.

Thanks for your report. I think your analysis is partially correct. If the
condition on line 216 evaluates to true then this means that the signature data
is just a bare OCTETSTRING. Really there should be an "else" on line 225 to
prevent the other branches from occurring. As it is the code will fall through
to the "else" on line 233 and it will then attempt to parse the signature data
as DigestInfo (X509_SIG). This will clearly fail because we already know that
it is a bare OCTETSTRING. Therefore |sig| will be NULL and we will goto the
|err| label - which is exactly where we would have ended up anyway had there
been an else on line 225 as I suggest.

So there is no real impact to this. You will never get to the signature
validity test on line 241 as you suggest if line 216 evaluates to true (and
that test would not apply anyway in this case). Functionally then it is working
correctly (kind of). However I have applied a patch anyway to prevent the
erroneous attempt to parse sig, and to make things look a bit more sane:

https://github.com/openssl/openssl/commit/dffe51091f412dcbc18f6641132f0b4f0def6bce

Closing this ticket.

Matt


> The related code snippets in int_rsa_verify() is as following.
> 168 int int_rsa_verify(int dtype, const unsigned char *m,
> 169 unsigned int m_len,
> 170 unsigned char *rm, size_t *prm_len,
> 171 const unsigned char *sigbuf, size_t siglen, RSA
> *rsa)
> 172 {
> 173 int i, ret = 0, sigtype;
> ...
> 216 if (dtype == NID_mdc2 && i == 18 && s[0] == 0x04 && s[1] ==
> 0x10) {
> 217 if (rm) {
> 218 memcpy(rm, s + 2, 16);
> 219 *prm_len = 16;
> 220 ret = 1;
> 221 } else if (memcmp(m, s + 2, 16))
> 222 RSAerr(RSA_F_INT_RSA_VERIFY, RSA_R_BAD_SIGNATURE);
> 223 else
> 224 ret = 1;
> 225 }
> 226
> 227 /* Special case: SSL signature */
> 228 if (dtype == NID_md5_sha1) {
> 229 if ((i != SSL_SIG_LENGTH) || memcmp(s, m, SSL_SIG_LENGTH))
> 230 RSAerr(RSA_F_INT_RSA_VERIFY, RSA_R_BAD_SIGNATURE);
> 231 else
> 232 ret = 1;
> 233 } else { // dtype != NID_md5_sha1
> 234 const unsigned char *p = s;
> 235 sig = d2i_X509_SIG(NULL, &p, (long)i);
> 236
> 237 if (sig == NULL)
> 238 goto err;
> 239
> 240 /* Excess data can be used to create forgeries */
> 241 if (p != s + i || !rsa_check_digestinfo(sig, s, i)) {
> 242 RSAerr(RSA_F_INT_RSA_VERIFY, RSA_R_BAD_SIGNATURE);
> 243 goto err;
> 244 }
> ...
> 283 err:
> 284 if (sig != NULL)
> 285 X509_SIG_free(sig);
> 286 if (s != NULL) {
> 287 OPENSSL_cleanse(s, (unsigned int)siglen);
> 288 OPENSSL_free(s);
> 289 }
> 290 return (ret);
> 291 }
>
>
>
>
>