aws / aws/s2n-tls

ensure s2n_alloc doesn't leak previous blobs

Open
#3,702 2 comments 0 reactions 0 assignees View on GitHub
good first issue priority/low s2n-core size/small type/cleanup type/refactor type/tech_debt
Dominant language
C
Stars
4.8k
Forks
802
Avg merge
5d 22h
Merged PRs (30d)
33

Description

### Problem:

`s2n_alloc` currently discards the previous blob argument, even if it might hold an allocation:

https://github.com/aws/s2n-tls/blob/2f9c7a48fd8faf44d5a6f375d6d3fbf9aada8e03/utils/s2n_mem.c#L150-L155

### Solution:

This should instead check that the blob is zeroed (or at least non-allocated) before proceeding:

```
diff --git a/utils/s2n_mem.c b/utils/s2n_mem.c
index fc0b3a8f..9cba6312 100644
--- a/utils/s2n_mem.c
+++ b/utils/s2n_mem.c
@@ -151,6 +151,9 @@ int s2n_alloc(struct s2n_blob *b, uint32_t size)
{
POSIX_ENSURE(initialized, S2N_ERR_NOT_INITIALIZED);
POSIX_ENSURE_REF(b);
+ POSIX_ENSURE(b->data == NULL, S2N_ERR_ALLOC);
+ POSIX_ENSURE(b->size == 0, S2N_ERR_ALLOC);
+ POSIX_ENSURE(b->allocated == 0, S2N_ERR_ALLOC);
const struct s2n_blob temp = { 0 };
*b = temp;
POSIX_GUARD(s2n_realloc(b, size));
```

### Requirements / Acceptance Criteria:

The tests should all pass after applying this change. You can see #2641 as an attempt to do this (I didn't get enough time to fix everything). There are several locations in the codebase where we pass an uninitialized blob to `s2n_alloc` and this causes problems of knowing if it's just garbage stack data or a legitimate previous blob being leaked.

I came up with a scrappy ripgrep script, although there are still some false-positives:

```
rg --sort path --line-number 'struct s2n_blob' . | rg -v '=' | rg -v 'struct s2n_blob \*' | rg -v 'struct s2n_blob\*' | rg -v '\.h'
```

[//]: # (NOTE: If you believe this might be a security issue, please email aws-security@amazon.com instead of creating a GitHub issue. For more details, see the AWS Vulnerability Reporting Guide: https://aws.amazon.com/security/vulnerability-reporting/ )

Contributor guide

Open the contributing guide

Research direction

Start in utils/s2n_mem.c at s2n_alloc around lines 150–155, then inspect the blob declarations found by the issue's ripgrep command. Review #2641 for prior work and identify callers passing uninitialized blobs. The change is done when previous allocations are not discarded and all tests pass.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
cryptography, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.