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

Add basic "security rules" #4298

Merged
merged 6 commits into from
Dec 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions stuffer/s2n_stuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#pragma once

#include <limits.h>
#include <stdarg.h>
#include <stdint.h>
#include <stdlib.h>
#include <sys/uio.h>
Expand Down Expand Up @@ -165,6 +166,16 @@ int s2n_stuffer_skip_read_until(struct s2n_stuffer *stuffer, const char *target)
int s2n_stuffer_alloc_ro_from_string(struct s2n_stuffer *stuffer, const char *str);
int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data, uint32_t length);

/* Stuffer versions of sprintf methods, except:
* - They write bytes, not strings. They do not write a final '\0'. Unfortunately,
* they do still require enough space for a final '\0'-- we'd have to reimplement
* sprintf to avoid that.
* - vprintf does not consume the vargs. It calls va_copy before using
* the varg argument, so can be called repeatedly with the same vargs.
*/
int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...);
int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs);
Comment on lines +169 to +177
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC all of the stuffer functions have CBMC proofs. It's probably good to have some for these new functions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely agree, but I think getting CBMC to work with varargs is going to be a bit of an adventure. I'd rather that didn't block this PR, particularly because the only direct manipulation of the stuffer is the "tainted" flag logic. Other than that, we just use existing stuffer functions.


/* Read a private key from a PEM encoded stuffer to an ASN1/DER encoded one */
int s2n_stuffer_private_key_from_pem(struct s2n_stuffer *pem, struct s2n_stuffer *asn1, int *type);

Expand Down
71 changes: 71 additions & 0 deletions stuffer/s2n_stuffer_text.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
*/

#include <string.h>
#include <sys/param.h>

#include "stuffer/s2n_stuffer.h"
#include "utils/s2n_mem.h"
Expand Down Expand Up @@ -206,3 +207,73 @@ int s2n_stuffer_init_ro_from_string(struct s2n_stuffer *stuffer, uint8_t *data,

return S2N_SUCCESS;
}

/* If we call va_start or va_copy there MUST be a matching call to va_end,
* so we should use DEFER_CLEANUP with our va_lists.
* Unfortunately, some environments implement va_list in ways that don't
* act as expected when passed by reference. For example, because va_end is
* a macro it may expect va_list to be an array (maybe to call sizeof),
* but passing va_list by reference will cause it to decay to a pointer instead.
* To avoid any surprises, just wrap the va_list in our own struct.
*/
struct s2n_va_list {
va_list va_list;
};
lrstewart marked this conversation as resolved.
Show resolved Hide resolved

static void s2n_va_list_cleanup(struct s2n_va_list *list)
{
if (list) {
va_end(list->va_list);
}
}

int s2n_stuffer_vprintf(struct s2n_stuffer *stuffer, const char *format, va_list vargs_in)
{
POSIX_PRECONDITION(s2n_stuffer_validate(stuffer));
POSIX_ENSURE_REF(format);

/* vsnprintf consumes the va_list, so copy it first */
DEFER_CLEANUP(struct s2n_va_list vargs_1 = { 0 }, s2n_va_list_cleanup);
va_copy(vargs_1.va_list, vargs_in);

/* The first call to vsnprintf calculates the size of the formatted string.
* str_len does not include the one byte vsnprintf requires for a trailing '\0',
* so we need one more byte.
*/
int str_len = vsnprintf(NULL, 0, format, vargs_1.va_list);
POSIX_ENSURE_GTE(str_len, 0);
int mem_size = str_len + 1;

/* 'tainted' indicates that pointers to the contents of the stuffer exist,
* so resizing / reallocated the stuffer will invalidate those pointers.
* However, we do no resize the stuffer in this method after creating `str`
* and `str` does not live beyond this method, so ignore `str` for the
* purposes of tracking 'tainted'.
*/
bool previously_tainted = stuffer->tainted;
char *str = s2n_stuffer_raw_write(stuffer, mem_size);
stuffer->tainted = previously_tainted;
lrstewart marked this conversation as resolved.
Show resolved Hide resolved
POSIX_GUARD_PTR(str);

/* vsnprintf again consumes the va_list, so copy it first */
DEFER_CLEANUP(struct s2n_va_list vargs_2 = { 0 }, s2n_va_list_cleanup);
va_copy(vargs_2.va_list, vargs_in);

/* This time, vsnprintf actually writes the formatted string */
int written = vsnprintf(str, mem_size, format, vargs_2.va_list);
POSIX_ENSURE_GTE(written, 0);

/* We don't actually use c-strings, so erase the final '\0' */
POSIX_GUARD(s2n_stuffer_wipe_n(stuffer, 1));
goatgoose marked this conversation as resolved.
Show resolved Hide resolved

POSIX_POSTCONDITION(s2n_stuffer_validate(stuffer));
return S2N_SUCCESS;
}

int s2n_stuffer_printf(struct s2n_stuffer *stuffer, const char *format, ...)
{
DEFER_CLEANUP(struct s2n_va_list vargs = { 0 }, s2n_va_list_cleanup);
va_start(vargs.va_list, format);
POSIX_GUARD(s2n_stuffer_vprintf(stuffer, format, vargs.va_list));
return S2N_SUCCESS;
}
Loading
Loading