[gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno created a merge request:

Project:Branches: dueno/gnutls:wip/dueno/reallocarray to gnutls/gnutls:master
Author: Daiki Ueno
Assignees:

Fixes: #1179

Checklist

  • Commits have Signed-off-by: with name/author being identical to the commit author
  • Code modified for feature
  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Documentation updated / NEWS entry present (for non-trivial changes)
  • CI timeout is 2h or higher (see Settings/CICD/General pipelines/Timeout)

Reviewer's checklist:

  • Any issues marked for closing are addressed
  • There is a test suite reasonably covering new functionality or modifications
  • Function naming, parameters, return values, types, etc., are consistent and according to CONTRIBUTION.md
  • This feature/change has adequate documentation added
  • No obvious mistakes in the code

_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
GitLab

Andreas Metzler commented:

Hello, I do not think you can use reallocarray in the library, is is "LGPL", not LGPLv2+. cu Andreas


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion:

Yes, I already sent a relicense request to the author. Even if it doesn't happen, the shim should be trivial to implement (as we have in p11-kit).


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion:

For the meantime I replaced it with a simple guard before calling realloc.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

All discussions on Merge Request !1392 were resolved by Daiki Ueno


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab
Merge request !1392 was reviewed by Stanislav Židek

Stanislav Židek started a new discussion on lib/x509/crl.c:

1276
-		    gnutls_realloc_fast(*crls,
1277
-					sizeof(gnutls_x509_crl_t) * init);
1275
+		*crls = _gnutls_reallocarray_fast(*crls, init,

If I understand correctly, the _fast version does also frees the original array if reallocation fails. What I don't fully understand is why e.g. this line uses _gnutls_reallocarray_fast, but line 1265 does not. Could you clarify?

Stanislav Židek started a new discussion on lib/x509/pkcs12.c:

1461
-						++(*chain_len));
1458
+			*chain = _gnutls_reallocarray_fast(*chain,
1459
+							   ++(*chain_len),

This is inconsistent with other places that do X + 1 most of the time (instead of ++X). Shall we make it consistent?

Stanislav Židek started a new discussion on lib/x509/ocsp.c:

2461
-			new_ocsps = gnutls_realloc(*ocsps, (*size + 1)*sizeof(gnutls_ocsp_resp_t));
2460
+			new_ocsps = _gnutls_reallocarray(*ocsps,
2461
+							 *size + 1,

One generic and theoretical question: I was told at the university that allocations are pretty expensive operations and we should not use them to expand arrays only by one item. I bet there is a pretty good reason for this in multiple places here, could you perhaps explain?

Stanislav Židek started a new discussion on lib/cert-cred.c:

66
-					 sizeof(certs_st));
64
+	res->certs = _gnutls_reallocarray_fast(res->certs,
65
+					       1 + res->ncerts,

Inconsisten with the rest of the code here and on line 59 - 1 + X vs. X + 1.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stanislav Židek commented:

See my review. I also checked other use of gnutls_malloc and found couple of examples with some arithmetic in arguments (mostly adding and multiplying by a constant). I suppose we don't want to be this paranoid, but wanted to raise this just in case.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/x509/crl.c:

1272 1272
 	    gnutls_x509_crl_list_import(*crls, &init, data, format,
1273 1273
 					flags | GNUTLS_X509_CRT_LIST_IMPORT_FAIL_IF_EXCEED);
1274 1274
 	if (ret == GNUTLS_E_SHORT_MEMORY_BUFFER) {
1275
-		*crls =
1276
-		    gnutls_realloc_fast(*crls,
1277
-					sizeof(gnutls_x509_crl_t) * init);
1275
+		*crls = _gnutls_reallocarray_fast(*crls, init,

In this specific case, line 1265 is the first time where *crls is allocated so there is no need for freeing anything.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/x509/ocsp.c:

2458 2457
 				goto fail;
2459 2458
 			}
2460 2459
 
2461
-			new_ocsps = gnutls_realloc(*ocsps, (*size + 1)*sizeof(gnutls_ocsp_resp_t));
2460
+			new_ocsps = _gnutls_reallocarray(*ocsps,
2461
+							 *size + 1,

I suppose there is a trade-off (i.e. alloc cost vs the frequency of the function call), but this specific case is about realloc, which should already have an optimization underneath to avoid frequent malloc-copy-free.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/x509/pkcs12.c:

1455 1455
 			    != 0)
1456 1456
 				goto skip;
1457 1457
 
1458
-			*chain =
1459
-			    gnutls_realloc_fast(*chain,
1460
-						sizeof((*chain)[0]) *
1461
-						++(*chain_len));
1458
+			*chain = _gnutls_reallocarray_fast(*chain,
1459
+							   ++(*chain_len),

I guess that's because we need to update *chain_len anyway here, while in other places we don't.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

All discussions on Merge Request !1392 were resolved by Daiki Ueno


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stanislav Židek commented on a discussion on lib/x509/pkcs12.c:

1455 1455
 			    != 0)
1456 1456
 				goto skip;
1457 1457
 
1458
-			*chain =
1459
-			    gnutls_realloc_fast(*chain,
1460
-						sizeof((*chain)[0]) *
1461
-						++(*chain_len));
1458
+			*chain = _gnutls_reallocarray_fast(*chain,
1459
+							   ++(*chain_len),

I see, my bad.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Stanislav Židek commented on a discussion on lib/cert-cred.c:

56 56
 				       int nr)
57 57
 {
58
-	res->sorted_cert_idx = gnutls_realloc_fast(res->sorted_cert_idx,
59
-						(1 + res->ncerts) *
60
-						sizeof(unsigned int));
58
+	res->sorted_cert_idx = _gnutls_reallocarray_fast(res->sorted_cert_idx,
59
+							 1 + res->ncerts,
60
+							 sizeof(unsigned int));
61 61
 	if (res->sorted_cert_idx == NULL)
62 62
 		return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR);
63 63
 
64
-	res->certs = gnutls_realloc_fast(res->certs,
65
-					 (1 + res->ncerts) *
66
-					 sizeof(certs_st));
64
+	res->certs = _gnutls_reallocarray_fast(res->certs,
65
+					       1 + res->ncerts,

Is there a reason why 1 + res->ncerts is better than res->ncerts + 1?


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion on lib/cert-cred.c:

56 56
 				       int nr)
57 57
 {
58
-	res->sorted_cert_idx = gnutls_realloc_fast(res->sorted_cert_idx,
59
-						(1 + res->ncerts) *
60
-						sizeof(unsigned int));
58
+	res->sorted_cert_idx = _gnutls_reallocarray_fast(res->sorted_cert_idx,
59
+							 1 + res->ncerts,
60
+							 sizeof(unsigned int));
61 61
 	if (res->sorted_cert_idx == NULL)
62 62
 		return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR);
63 63
 
64
-	res->certs = gnutls_realloc_fast(res->certs,
65
-					 (1 + res->ncerts) *
66
-					 sizeof(certs_st));
64
+	res->certs = _gnutls_reallocarray_fast(res->certs,
65
+					       1 + res->ncerts,

The follow-up change actually makes it consistent with the latter style.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

All discussions on Merge Request !1392 were resolved by Daiki Ueno


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel
Reply | Threaded
Open this post in threaded view
|

Re: [gnutls-devel] GnuTLS | build: avoid potential integer overflow in array allocation (!1392)

Read-only notification of GnuTLS library development activities
In reply to this post by Read-only notification of GnuTLS library development activities
GitLab

Daiki Ueno commented on a discussion:

This is a good point; we should probably use xsum and size_overflow_p for the addtions as well.


_______________________________________________
Gnutls-devel mailing list
[hidden email]
http://lists.gnupg.org/mailman/listinfo/gnutls-devel