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

[Read-only notification of GnuTLS library development activities]

Merge request https://gitlab.com/gnutls/gnutls/-/merge_requests/1367 was reviewed by Daiki Ueno

--
  
Daiki Ueno started a new discussion on lib/x509/x509_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965196

> +{
> +	switch (sigalg) {
> +	case GNUTLS_SIGN_RSA_MD5:

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

--
  
Daiki Ueno started a new discussion on lib/x509/x509_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965197

> +	memcpy(sct->logid, ptr, SCT_V1_LOGID_SIZE);
> +	ptr += SCT_V1_LOGID_SIZE;
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965198

> +	 * Check that there are actually no extensions - the following two bytes should be zero.
> +	 */
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965200

> +	length -= 2;
> +
> +	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:
```c
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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965202

> +
> +	/* Signature, length and content */
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965203

> +	struct ct_sct_st *new_scts;
> +
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965207

> +}
> +
> +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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965209

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

Missing `length >= 2` check?

--
  
Daiki Ueno started a new discussion on lib/x509/x509_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965210

> +	}
> +
> +	gnutls_free(scts_content.data);

nit: `_gnutls_free_datum(&scts_content)`.

--
  
Daiki Ueno started a new discussion on lib/x509/x509_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965211

> +struct gnutls_x509_ct_scts_st {
> +	struct ct_sct_st *scts;
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965212

> +
> +cleanup:
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965213

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

No predefined error code?

--
  
Daiki Ueno started a new discussion on lib/x509/x509_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965214

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

No predefined error code?

--
  
Daiki Ueno started a new discussion on lib/x509/x509_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965215

> +					   sct->signature.data,
> +					   sct->signature.size);
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965216

> +					   sct->logid,
> +					   SCT_V1_LOGID_SIZE);
> +	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: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367#note_533965217

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

nit: no need for parentheses in RHS.


-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/-/merge_requests/1367
You're receiving this email because of your account on gitlab.com.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnutls-devel/attachments/20210320/3197dcbc/attachment-0001.html>