Re: [gnutls-devel] GnuTLS | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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

Re: [gnutls-devel] GnuTLS | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

Read-only notification of GnuTLS library development activities
GitLab

Ander Juaristi changed the draft status of merge request !1367


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

Read-only notification of GnuTLS library development activities
GitLab

Assignee changed to Ander Juaristi


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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 !1367 was reviewed by Daiki Ueno

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3615
+{
3616
+	switch (sigalg) {
3617
+	case GNUTLS_SIGN_RSA_MD5:

Is this code path performance critical? If not, I would suggest using a table (variable) instead of switchs in this function and get_sigalg*.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3699
+	memcpy(sct->logid, ptr, SCT_V1_LOGID_SIZE);
3700
+	ptr += SCT_V1_LOGID_SIZE;
3701
+	length -= SCT_V1_LOGID_SIZE;

Although I don't see any use of it under lib/x509, perhaps we could use DECR_LENGTH_RET?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3716
+	 * Check that there are actually no extensions - the following two bytes should be zero.
3717
+	 */
3718
+	if (*ptr != 0 || *(ptr+1) != 0) {

Shouldn't we check length >= 2 before dereferencing ptr and ptr + 1?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3741
+	length -= 2;
3742
+
3743
+	sct->sigalg = get_sigalg(hash_algo, sig_algo);

get_sigalg can return an error code outside of the gnutls_signature_algorithm_t enum range, so I suspect some static analyzers would complain this part. I suggest rewriting this to:

ret = get_sigalg(hash_algo, sig_algo);
if (ret < 0) {
        goto cleanup;
}
sct->sigalg = ret;

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3749
+
3750
+	/* Signature, length and content */
3751
+	sig_length = _gnutls_read_uint16(ptr);

Missing length >= 2 check before accessing ptr?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3788
+	struct ct_sct_st *new_scts;
3789
+
3790
+	new_scts = gnutls_realloc(*scts, (*size + 1) * sizeof(struct ct_sct_st));

Missing overflow check in multiplication (see !1392).

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3799
+}
3800
+
3801
+static int _gnutls_export_ct_v1_sct(const struct ct_sct_st *sct, size_t base_size, uint8_t *out)

Can we assume that out always has enough room for writing? If so, you could add a comment why we can omit bounds-check in this function.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3871
+	ptr = &scts_content.data[2];
3872
+	while (length > 0) {
3873
+		sct_length = _gnutls_read_uint16(ptr);

Missing length >= 2 check?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3887
+	}
3888
+
3889
+	gnutls_free(scts_content.data);

nit: _gnutls_free_datum(&scts_content).

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3477
+struct gnutls_x509_ct_scts_st {
3478
+	struct ct_sct_st *scts;
3479
+	unsigned int size;

I suggest using size_t everywhere for sizes, unless there is a good reason.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3951
+
3952
+cleanup:
3953
+	if (out)

No need for this if, if you initialize out as NULL.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3970
+	version = scts->scts[idx].version;
3971
+	if (version != 0 || version_out == NULL)
3972
+		return -1;

No predefined error code?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3990
+	sct = &scts->scts[idx];
3991
+	if (sct->version != 0)
3992
+		return -1;

No predefined error code?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3995
+					   sct->signature.data,
3996
+					   sct->signature.size);
3997
+	if (retval < 0)

nit: what about moving this check inside the if (signature) { ... } block?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

4001
+					   sct->logid,
4002
+					   SCT_V1_LOGID_SIZE);
4003
+	if (retval < 0) {

nit: what about moving this check inside the if (logid) { ... } block?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

4006
+	}
4007
+	if (timestamp)
4008
+		*timestamp = (sct->timestamp / 1000);

nit: no need for parentheses in RHS.


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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 !1367 was reviewed by Daiki Ueno

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3705
+	 * In version 1, it has a fixed length of 32 bytes.
3706
+	 */
3707
+	if (length <= SCT_V1_LOGID_SIZE) {

Why this comparison is <=, not <?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3716
+
3717
+	/* Timestamp */
3718
+	if (length <= sizeof(uint64_t)) {

Ditto, use <.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3779
+
3780
+	length -= sig_length;
3781
+	if (length) {

Check length > sig_length before subtracting; otherwise unsigned arithmetic wraps around.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3873
+ * Returns: %GNUTLS_E_SUCCESS (0) on success or a negative error value.
3874
+ **/
3875
+int gnutls_x509_ext_ct_import_scts(const gnutls_datum_t *ext, gnutls_x509_ct_scts_t scts)

I suggest swapping the order of arguments, to match other _import functions.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3889
+
3890
+	length = _gnutls_read_uint16(scts_content.data);
3891
+	if (length <= 4) {

Use <.

Daiki Ueno started a new discussion on tests/x509cert-ct.c:

264
+void doit(void)
265
+{
266
+	int ret, scts_printed = 0;

nit: better use bool for scts_printed.


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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

Ander Juaristi commented on a discussion on lib/x509/x509_ext.c:

3703
+	/* Timestamp */
3704
+	if (length <= sizeof(uint64_t)) {
3705
+		gnutls_assert();
3706
+		ret = GNUTLS_E_PREMATURE_TERMINATION;
3707
+		goto cleanup;
3708
+	}
3709
+
3710
+	sct->timestamp = (uint64_t) _gnutls_read_uint64(ptr);
3711
+	ptr += sizeof(uint64_t);
3712
+	length -= sizeof(uint64_t);
3713
+
3714
+	/*
3715
+	 * There are no extensions defined in SCT v1.
3716
+	 * Check that there are actually no extensions - the following two bytes should be zero.
3717
+	 */
3718
+	if (*ptr != 0 || *(ptr+1) != 0) {

I've rewritten it as:

if (length < 2 || *ptr != 0 || *(ptr+1) != 0) {
    gnutls_assert();
    ret = GNUTLS_E_UNEXPECTED_EXTENSIONS_LENGTH;
    goto cleanup;
}

_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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

Ander Juaristi commented on a discussion on lib/x509/x509_ext.c:

3766
+	ptr += sizeof(uint16_t);
3767
+	length -= sizeof(uint16_t);
3768
+	if (sig_length == 0) {
3769
+		gnutls_assert();
3770
+		ret = GNUTLS_E_PREMATURE_TERMINATION;
3771
+		goto cleanup;
3772
+	}
3773
+
3774
+	if (_gnutls_set_datum(&sct->signature, ptr, sig_length) < 0) {
3775
+		gnutls_assert();
3776
+		ret = GNUTLS_E_MEMORY_ERROR;
3777
+		goto cleanup;
3778
+	}
3779
+
3780
+	length -= sig_length;
3781
+	if (length) {

I've rewritten it as:

if (length != sig_length) {
    gnutls_assert();
    ret = GNUTLS_E_ASN1_DER_OVERFLOW;
    goto cleanup;
}

_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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 !1367 was reviewed by Daiki Ueno

Daiki Ueno started a new discussion on lib/x509/output.c:

1227 1320
 
1228 1321
 		print_aia(str, der);
1322
+	} else if (strcmp(oid, "1.3.6.1.4.1.11129.2.4.2") == 0) {

What about using GNUTLS_X509EXT_OID_CT_SCT?

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3745
+	 * and return them as a gnutls_sign_algorithm_t enum value.
3746
+	 */
3747
+	if (length <= 2) {

Use <.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3887
+		return gnutls_assert_val(retval);
3888
+
3889
+	length = _gnutls_read_uint16(scts_content.data);

Check scts_content.size >= 2.

Daiki Ueno started a new discussion on lib/x509/x509_ext.c:

3896
+	while (length > 0) {
3897
+		sct_length = _gnutls_read_uint16(ptr);
3898
+		if (sct_length == 0 || sct_length > length)

I think sct_length == 0 indicates the end of data, while sct_length > length seems to be an error. Maybe better handle them separately?


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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

Ander Juaristi commented on a discussion on lib/x509/x509_ext.c:

3775
+		goto cleanup;
3776
+	}
3777
+
3778
+	return 0;
3779
+
3780
+cleanup:
3781
+	_gnutls_free_datum(&sct->signature);
3782
+	return ret;
3783
+}
3784
+
3785
+static int _gnutls_ct_sct_add(struct ct_sct_st *sct,
3786
+			      struct ct_sct_st **scts, unsigned int *size)
3787
+{
3788
+	struct ct_sct_st *new_scts;
3789
+
3790
+	new_scts = gnutls_realloc(*scts, (*size + 1) * sizeof(struct ct_sct_st));

Okay, but wouldn't it be better to wait until !1392 is merged? Right now I don't see any _gnutls_reallocarray.


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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:

Thanks for the update. I am not sure if I can manually spot all the missing bounds checks; on the other hand, if we overlook any, that would lead to a security issue. I've filed a proposal doing that in a safer way (#1194).


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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/x509_ext.c:

3602
+		return get_sigalg_for_sha1(sig_algo);
3603
+	case 3: /* sha224 */
3604
+		return get_sigalg_for_sha224(sig_algo);
3605
+	case 4: /* sha256 */
3606
+		return get_sigalg_for_sha256(sig_algo);
3607
+	case 5: /* sha384 */
3608
+		return get_sigalg_for_sha384(sig_algo);
3609
+	case 6: /* sha512 */
3610
+		return get_sigalg_for_sha512(sig_algo);
3611
+	}
3612
+}
3613
+
3614
+static int write_sigalg(gnutls_sign_algorithm_t sigalg, uint8_t *out)
3615
+{
3616
+	switch (sigalg) {
3617
+	case GNUTLS_SIGN_RSA_MD5:

I was thinking something like below, similar to the arrays defined in lib/algorithms/*:

struct sct_sign_algorithm_st {
  uint8_t codepoint[2];
  gnutls_sign_algorithm_t sign_algo;
  gnutls_hash_algorithm_t hash_algo;
};

const struct sct_sign_algorithm_st[] = {
  ...
  {
    .codepoint = { 0x05, 0x01 },
    .sign_algo = GNUTLS_SIGN_RSA_SHA384,
    .hash_algo = GNUTLS_DIG_SHA384,
  },
  ...
};

It would be slower than the accessing 2-dimensional matrix, as it requires searching on the array, but given the fact that there is only 18 (= 6 * 3) elements, that wouldn't affect performance much.


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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

Ander Juaristi commented on a discussion on lib/x509/x509_ext.c:

3860
+
3861
+/**
3862
+ * gnutls_x509_ext_ct_import_scts:
3863
+ * @ext: a DER-encoded extension
3864
+ * @scts: The SCT list
3865
+ *
3866
+ * This function will read a SignedCertificateTimestampList structure
3867
+ * from the DER data of the X.509 Certificate Transparency SCT extension
3868
+ * (OID 1.3.6.1.4.1.11129.2.4.2).
3869
+ *
3870
+ * The list of SCTs (Signed Certificate Timestamps) is placed on @scts,
3871
+ * which must be previously initialized with gnutls_x509_ext_ct_scts_init().
3872
+ *
3873
+ * Returns: %GNUTLS_E_SUCCESS (0) on success or a negative error value.
3874
+ **/
3875
+int gnutls_x509_ext_ct_import_scts(const gnutls_datum_t *ext, gnutls_x509_ct_scts_t scts)

I'm seeing in other _import functions the data comes last, but this function follows the order of the other _import functions defined in x509_ext.c. Looks like in the functions that read X.509 extensions the data (ext) comes first.


_______________________________________________
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 | Read Certificate Transparency (RFC 6962) SCT extension (!1367)

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

Ander Juaristi commented on a discussion:

That's great. I'll have a closer look anyway now.


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