Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverity Fixes #12531

Closed
Closed

Conversation

@ashman-p
Copy link
Contributor

@ashman-p ashman-p commented Jul 25, 2020

ecdh_kdf.c: Improper use of negative value
x_algor .c: Explicit null dereferenced
cms_sd .c: Resource leak
extensions_srvr.c : Resource leak
pk7_smime.c: String not null terminated
ts_rsp_sign.c Resource Leak
asn_mime.c: Explicit null dereferenced
extensions_srvr.c: Resourse Leak
v3_alt.c: Resourse Leak
pcy_data.c: Resource Leak
cms_lib.c: Resource Leak
drbg_lib.c: Unchecked return code

Fixes #12529

Checklist
  • documentation is added or updated
  • tests are added or updated
crypto/asn1/asn_mime.c Outdated Show resolved Hide resolved
crypto/cms/cms_lib.c Outdated
if (!icont)
BIO_free(cont);
return NULL;
Comment on lines 95 to 97

This comment has been minimized.

@slontis

slontis Jul 26, 2020
Contributor

just do

goto err;

add add label at bottom for the identical code.

err:

This should also be fixed in master

This comment has been minimized.

@ashman-p

ashman-p Jul 28, 2020
Author Contributor

Changed.

crypto/ec/ecdh_kdf.c Outdated
@@ -58,7 +58,9 @@ int ecdh_KDF_X9_63(unsigned char *out, size_t outlen,
if (!EVP_DigestFinal(mctx, mtmp, NULL))
goto err;
memcpy(out, mtmp, outlen);
OPENSSL_cleanse(mtmp, mdlen);
if (mdlen > 0) {

This comment has been minimized.

@slontis

slontis Jul 26, 2020
Contributor

I would not do this check here..

move the

mdlen = EVP_MD_size(md);

line above the new and then do a negative there and return 0 instead.

This comment has been minimized.

@ashman-p

ashman-p Jul 28, 2020
Author Contributor

Changed.

crypto/pkcs7/pk7_smime.c Outdated
@@ -322,7 +322,7 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store,
tmpout = out;

/* We now have to 'read' from p7bio to calculate digests etc. */
if ((buf = OPENSSL_malloc(BUFFERSIZE)) == NULL) {
if ((buf = OPENSSL_zalloc(BUFFERSIZE)) == NULL) {

This comment has been minimized.

@slontis

slontis Jul 26, 2020
Contributor

not sure I understand why this needs to be done here

This comment has been minimized.

@ashman-p

ashman-p Jul 27, 2020
Author Contributor

The Coverity issue was "String not null terminated". Looking at the details more closely... the bio callbacks used for read and write ends up using strlen which expects the null terminated buffer.
I think we can ignore this one and have it fixed in the callback.

crypto/rand/drbg_lib.c Outdated Show resolved Hide resolved
crypto/ts/ts_rsp_sign.c Outdated
@@ -55,8 +55,10 @@ static ASN1_INTEGER *def_serial_cb(struct TS_resp_ctx *ctx, void *data)

if (serial == NULL)
goto err;
if (!ASN1_INTEGER_set(serial, 1))
if (!ASN1_INTEGER_set(serial, 1)) {

This comment has been minimized.

@slontis

slontis Jul 26, 2020
Contributor

Move the

ASN1_INTEGER_free(serial);

to the err block (it does this in master)

This comment has been minimized.

@ashman-p

ashman-p Jul 28, 2020
Author Contributor

Changed.

ssl/statem/extensions_srvr.c Outdated
@@ -1151,6 +1151,7 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
if (sesstmp == NULL) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR,
SSL_F_TLS_PARSE_CTOS_PSK, ERR_R_INTERNAL_ERROR);
SSL_SESSION_free(sess);

This comment has been minimized.

@slontis

slontis Jul 26, 2020
Contributor

use this instead..
goto err:

Needs to go back to master..

This comment has been minimized.

@ashman-p

ashman-p Jul 28, 2020
Author Contributor

Changed.

Copy link
Contributor

@slontis slontis left a comment

Thanks for fixing coverity errors.

Any relevant ones that I have listed need to be fixed in master before this PR for 111 will be merged.
Would you mind creating a seperate PR against master an then provide a reference to it in this PR.

ashman-p added a commit to ashman-p/openssl that referenced this pull request Jul 29, 2020
@ashman-p ashman-p mentioned this pull request Jul 29, 2020
0 of 2 tasks complete
@ashman-p
Copy link
Contributor Author

@ashman-p ashman-p commented Jul 29, 2020

Thanks for fixing coverity errors.

Any relevant ones that I have listed need to be fixed in master before this PR for 111 will be merged.
Would you mind creating a seperate PR against master an then provide a reference to it in this PR.

Glad to do it. the PR for master was created.
Thanks,
Norman

@ashman-p
Copy link
Contributor Author

@ashman-p ashman-p commented Jul 29, 2020

Looks like the test failure is due to a bad cert?

openssl x509 -in test/certs/ee-self-signed.pem -noout -text

Certificate:
Data:
Version: 3 (0x2)
Serial Number:
04:fe:e2:10:a3:e5:2a:e8:a7:64:64:0d:17:14:98:dc:80:48:6f:4b
Signature Algorithm: sha256WithRSAEncryption
Issuer: CN=ee-self-signed
Validity
Not Before: Jun 28 10:51:45 2020 GMT
Not After : Jul 28 10:51:45 2020 GMT
^^^^^^^^^^^^^^^^^^

@slontis
Copy link
Contributor

@slontis slontis commented Jul 29, 2020

A rebase will fix this.. It has been fixed by @mattcaswell in merged PR #12545

@ashman-p
Copy link
Contributor Author

@ashman-p ashman-p commented Jul 30, 2020

A rebase will fix this.. It has been fixed by @mattcaswell in merged PR #12545

This does not appear to be applied to 1.1.1 yet.

@mattcaswell
Copy link
Member

@mattcaswell mattcaswell commented Jul 30, 2020

I think @slontis referred to the wrong PR. He probably meant #12549.

It wasn't backported because I hadn't realised the issue also affected the 1.1.1 branch. I've created a new backport PR In #12561.

ashman-p added 3 commits Jul 25, 2020
ecdh_kdf.c: Improper use of negative value
 x_algor .c: Explicit null dereferenced
cms_sd .c: Resource leak
extensions_srvr.c : Resource leak
pk7_smime.c: String not null terminated
ts_rsp_sign.c Resource Leak
asn_mime.c: Explicit null dereferenced
extensions_srvr.c: Resourse Leak
v3_alt.c: Resourse Leak
pcy_data.c: Resource Leak
cms_lib.c: Resource Leak
drbg_lib.c: Unchecked return code
Updates per code review.
Revert files per PR for coverity issues.
@ashman-p ashman-p force-pushed the ashman-p:coverity-fixes branch to 50a8ba4 Jul 31, 2020
ashman-p added 2 commits Jul 31, 2020
crypto/ec/ecdh_kdf.c:36:5: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits]
     if (mdlen < 0)
     ^
This coverity issus is no longer a problem now that EVP_MD_size() can no longer return -1.
Removing ecdh_kdf.c from the PR.
@slontis
Copy link
Contributor

@slontis slontis commented Aug 4, 2020

Changes have been applied to master. So the hold has been removed.

Copy link
Contributor

@slontis slontis left a comment

LGTM

@slontis
Copy link
Contributor

@slontis slontis commented Aug 4, 2020

ping

@slontis
slontis approved these changes Aug 4, 2020
Copy link
Contributor

@slontis slontis left a comment

LGTM

@slontis
Copy link
Contributor

@slontis slontis commented Aug 10, 2020

ping

Copy link
Member

@mattcaswell mattcaswell left a comment

Approved if the nit already exists in master.

@@ -554,7 +554,9 @@ int rand_drbg_restart(RAND_DRBG *drbg,
drbg->meth->reseed(drbg, adin, adinlen, NULL, 0);
} else if (reseeded == 0) {
/* do a full reseeding if it has not been done yet above */
RAND_DRBG_reseed(drbg, NULL, 0, 0);
if(!RAND_DRBG_reseed(drbg, NULL, 0, 0)) {

This comment has been minimized.

@mattcaswell

mattcaswell Aug 10, 2020
Member

There's a formatting nit here. Should be if (, i.e. with a space after if and before opening parens. Is this already like this on master? If so then possibly leave it.

This comment has been minimized.

@paulidale

paulidale Aug 10, 2020
Contributor

drbg_lib.c no longer exists on master.

swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
Fixes openssl#12531 on master branch.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from openssl#12557)
slontis added 2 commits Sep 7, 2020
@slontis
Copy link
Contributor

@slontis slontis commented Sep 7, 2020

fixed some spacing NITS

openssl-machine pushed a commit that referenced this pull request Sep 7, 2020
x_algor.c: Explicit null dereferenced
cms_sd.c: Resource leak
ts_rsp_sign.c Resource Leak
extensions_srvr.c: Resourse Leak
v3_alt.c: Resourse Leak
pcy_data.c: Resource Leak
cms_lib.c: Resource Leak
drbg_lib.c: Unchecked return code

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from #12531)
@slontis
Copy link
Contributor

@slontis slontis commented Sep 7, 2020

Thanks for fixing..
Merged to 111.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants