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

Adds CBMC proof for sign_header #659

Merged
merged 24 commits into from
Jan 11, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
533c105
Split aws_cryptosdk_hdr_parse into smaller functions
alex-chew Nov 26, 2020
183bd77
Merge branch 'master' into split-hdr-parse
alex-chew Nov 26, 2020
855217a
Merge branch 'master' into split-hdr-parse
alex-chew Dec 1, 2020
57a97cd
merge
tegansb Dec 7, 2020
9cbe91e
Adds harness, Makefile and yaml for sign_header
tegansb Dec 8, 2020
f128327
Added preconditions to source code and added checks for memcpy
tegansb Dec 8, 2020
318601b
Update Makefile
tegansb Dec 10, 2020
3fee097
Remove unneeded empty slot assumption
tegansb Dec 10, 2020
d128aef
Merge branch 'master' into sign_header
alex-chew Dec 15, 2020
41c671f
Merge branch 'master' into sign_header
tegansb Dec 16, 2020
54135ed
Use new hdr_setup in harness and fix comment
tegansb Dec 18, 2020
8d8a1b5
Added comment about when the object-bits flag is required
tegansb Dec 20, 2020
1c0ea26
Added preconditions of lne of byte bufs
tegansb Dec 21, 2020
038c3da
Use CBMC_OBJECT_BITS flag
tegansb Dec 21, 2020
beb8a1e
Make len != 0 check explicit
tegansb Dec 21, 2020
9a628e1
Merge branch 'master' into sign_header
tegansb Dec 22, 2020
99b7aa9
Merge branch 'master' into sign_header
tegansb Dec 24, 2020
26db5d3
Use session_setup and aws_session_is_valid
tegansb Dec 29, 2020
76e8d41
Merge branch 'master' into sign_header
adpaco-aws Jan 6, 2021
c06ea6a
Merge branch 'master' into sign_header
adpaco-aws Jan 8, 2021
4d0477a
Remove signctx precondition in harness. Weaken precondition in EVPDig…
adpaco-aws Jan 11, 2021
0dd0ee0
Stub out hdr_write. Use default CBMC_OBJECT_BITS value
adpaco-aws Jan 11, 2021
56621c2
Remove unwind value for hdr_write
adpaco-aws Jan 11, 2021
e537227
Add and improve documentation
adpaco-aws Jan 11, 2021
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
32 changes: 27 additions & 5 deletions source/session_encrypt.c
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,12 @@ static int build_header(struct aws_cryptosdk_session *session, struct aws_crypto
}

static int sign_header(struct aws_cryptosdk_session *session) {
AWS_PRECONDITION(session != NULL);
AWS_PRECONDITION(aws_allocator_is_valid(session->alloc));
AWS_PRECONDITION(aws_cryptosdk_hdr_is_valid(&session->header));
AWS_PRECONDITION(aws_cryptosdk_sig_ctx_is_valid(session->signctx));
AWS_PRECONDITION(session->state == ST_GEN_KEY);
AWS_PRECONDITION(session->mode == AWS_CRYPTOSDK_ENCRYPT);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace it by

AWS_PRECONDITION(aws_cryptosdk_session_is_valid(session));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

session->header_size = aws_cryptosdk_hdr_size(&session->header);

if (session->header_size == 0) {
Expand All @@ -183,8 +189,14 @@ static int sign_header(struct aws_cryptosdk_session *session) {
// see what happened. It also makes sure that the header is fully initialized,
// again just in case some bug doesn't overwrite them properly.

memset(session->header.iv.buffer, 0x42, session->header.iv.len);
memset(session->header.auth_tag.buffer, 0xDE, session->header.auth_tag.len);
if (session->header.iv.len) {
assert(session->header.iv.buffer);
memset(session->header.iv.buffer, 0x42, session->header.iv.len);
}
if (session->header.auth_tag.len) {
assert(session->header.auth_tag.buffer);
memset(session->header.auth_tag.buffer, 0xDE, session->header.auth_tag.len);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we assert or return error? @alex-chew

Copy link
Contributor

Choose a reason for hiding this comment

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

Short answer: assert. The condition should always be true, and if not then it's a programming error rather than a user/data error.

Long answer: I think we can actually leave out the assert, since the aws_cryptosdk_hdr_is_valid(&session->header) precondition implies that the session->header.iv and session->header.auth_tag are aws_byte_buf_is_valid, which in turn implies that iv.len implies iv.buffer and likewise for auth_tag. But the assert is valuable as documentation of this intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Digging deeper, it looks like the only callsite of sign_header appears after a successfull call to build_header, which initializes these buffers to have positive length (since all supported algorithm suites use 12- and 16-byte IVs and authentication tag lengths, respectively). So we can change the guards/asserts to preconditions (i.e. byte_buf_is_valid and buf.len > 0 for each of the IV and auth tag).


size_t actual_size;

Expand All @@ -205,10 +217,20 @@ static int sign_header(struct aws_cryptosdk_session *session) {
if (rv) return AWS_OP_ERR;

if (session->alg_props->msg_format_version == AWS_CRYPTOSDK_HEADER_VERSION_1_0) {
memcpy(session->header.iv.buffer, authtag.buffer, session->header.iv.len);
memcpy(session->header.auth_tag.buffer, authtag.buffer + session->header.iv.len, session->header.auth_tag.len);
if (session->header.iv.len) {
tegansb marked this conversation as resolved.
Show resolved Hide resolved
assert(session->header.iv.buffer);
memcpy(session->header.iv.buffer, authtag.buffer, session->header.iv.len);
}
if (session->header.auth_tag.len) {
assert(session->header.auth_tag.buffer);
memcpy(
session->header.auth_tag.buffer, authtag.buffer + session->header.iv.len, session->header.auth_tag.len);
}
} else {
memcpy(session->header.auth_tag.buffer, authtag.buffer, session->header.auth_tag.len);
if (session->header.auth_tag.len) {
assert(session->header.auth_tag.buffer);
memcpy(session->header.auth_tag.buffer, authtag.buffer, session->header.auth_tag.len);
}
tegansb marked this conversation as resolved.
Show resolved Hide resolved
}

// Re-serialize the header now that we know the auth tag
Expand Down
109 changes: 109 additions & 0 deletions verification/cbmc/proofs/sign_header/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use
# this file except in compliance with the License. A copy of the License is
# located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
# implied. See the License for the specific language governing permissions and
# limitations under the License.

# if Makefile.local exists, use it. This provides a way to override the defaults
sinclude ../Makefile.local
#otherwise, use the default values
include ../Makefile.local_default
include ../Makefile.aws_byte_buf

PROOF_UID = sign_header

HARNESS_ENTRY = $(PROOF_UID)_harness
HARNESS_FILE = $(HARNESS_ENTRY).c

# Increasing the length of the edk_list or enc_ctx does not improve coverage.
# We use these values for performance.
MAX_EDK_LIST_ITEMS ?= 1
MAX_TABLE_SIZE ?= 2
MAX_IV_LEN ?= 12

DEFINES += -DAWS_NO_STATIC_IMPL
DEFINES += -DMAX_EDK_LIST_ITEMS=$(MAX_EDK_LIST_ITEMS)
DEFINES += -DMAX_TABLE_SIZE=$(MAX_TABLE_SIZE)
DEFINES += -DARRAY_LIST_TYPE="struct aws_cryptosdk_edk"
DEFINES += -DARRAY_LIST_TYPE_HEADER=\"aws/cryptosdk/edk.h\"
DEFINES += -DMAX_IV_LEN=$(MAX_IV_LEN)
DEFINES += -DAWS_CRYPTOSDK_HASH_ELEMS_ARRAY_INIT_GENERATOR=array_list_item_generator

# The maximum number of objects is set to 2^8 = 256. This option changes that to 2^9.
# Without this flag, running this proof causes the CBMC error of
# "too many addressed objects: maximum number of objects is set to 2^n=256 (with n=8);
# use the `--object-bits n` option to increase the maximum number"
CBMCFLAGS += --object-bits 9
tegansb marked this conversation as resolved.
Show resolved Hide resolved

PROJECT_SOURCES += $(CBMC_ROOT)/aws-c-common/source/byte_buf.c
PROJECT_SOURCES += $(CBMC_ROOT)/aws-c-common/source/hash_table.c
PROJECT_SOURCES += $(CBMC_ROOT)/aws-c-common/source/string.c
PROJECT_SOURCES += $(COMMON_PROOF_UNINLINE)/array_list.c
PROJECT_SOURCES += $(COMMON_PROOF_UNINLINE)/byte_order.c
PROJECT_SOURCES += $(COMMON_PROOF_UNINLINE)/error.c
PROJECT_SOURCES += $(COMMON_PROOF_UNINLINE)/math.c
PROJECT_SOURCES += $(COMMON_PROOF_UNINLINE)/string.c
PROJECT_SOURCES += $(PROOF_SOURCE)/openssl/bn_override.c
PROJECT_SOURCES += $(PROOF_SOURCE)/openssl/ec_override.c
PROJECT_SOURCES += $(PROOF_SOURCE)/openssl/evp_override.c
PROJECT_SOURCES += $(SRCDIR)/source/cipher.c
PROJECT_SOURCES += $(SRCDIR)/source/cipher_openssl.c
PROJECT_SOURCES += $(SRCDIR)/source/edk.c
PROJECT_SOURCES += $(SRCDIR)/source/enc_ctx.c
PROJECT_SOURCES += $(SRCDIR)/source/header.c
PROJECT_SOURCES += $(SRCDIR)/source/session.c
PROJECT_SOURCES += $(SRCDIR)/source/session_encrypt.c
PROJECT_SOURCES += $(SRCDIR)/source/utils.c

PROOF_SOURCES += $(COMMON_PROOF_SOURCE)/make_common_data_structures.c
PROOF_SOURCES += $(COMMON_PROOF_SOURCE)/proof_allocators.c
PROOF_SOURCES += $(COMMON_PROOF_SOURCE)/utils.c
PROOF_SOURCES += $(COMMON_PROOF_STUB)/aws_array_list_defined_type.c
PROOF_SOURCES += $(COMMON_PROOF_STUB)/aws_byte_buf_write_stub.c
PROOF_SOURCES += $(COMMON_PROOF_STUB)/aws_hash_iter_overrides.c
Comment on lines +67 to +68
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you create a GitHub issue to standardize the name of stubs? Some of them use as sufix stub some others use overrides some other use something else... (no a blocker for this PR).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done! #686

PROOF_SOURCES += $(COMMON_PROOF_STUB)/error.c
PROOF_SOURCES += $(COMMON_PROOF_STUB)/memcmp_override.c
PROOF_SOURCES += $(COMMON_PROOF_STUB)/memcpy_override_havoc.c
PROOF_SOURCES += $(COMMON_PROOF_STUB)/memset_override_havoc.c
PROOF_SOURCES += $(PROOF_SOURCE)/cbmc_invariants.c
PROOF_SOURCES += $(PROOF_SOURCE)/make_common_data_structures.c
PROOF_SOURCES += $(PROOF_STUB)/aws_array_list_sort_noop_stub.c
PROOF_SOURCES += $(PROOF_STUB)/aws_cryptosdk_enc_ctx_size_stub.c
PROOF_SOURCES += $(PROOF_STUB)/aws_cryptosdk_hash_elems_array_init_stub.c
PROOF_SOURCES += $(PROOF_STUB)/aws_default_allocator_stub.c
PROOF_SOURCES += $(PROOFDIR)/$(HARNESS_FILE)

REMOVE_FUNCTION_BODY += aws_array_list_get_at_ptr
adpaco-aws marked this conversation as resolved.
Show resolved Hide resolved

# Remove function bodies to deal with CBMC function pointer removal
# All functions removed are those removed in aws_cryptosdk_enc_ctx_serialize
REMOVE_FUNCTION_BODIES += --remove-function-body aws_array_list_sort
REMOVE_FUNCTION_BODIES += --remove-function-body aws_byte_buf_write
REMOVE_FUNCTION_BODIES += --remove-function-body aws_cryptosdk_enc_ctx_size
REMOVE_FUNCTION_BODIES += --remove-function-body aws_cryptosdk_hash_elems_array_init

# We abstract these functions because manual inspection demonstrates that they are unreachable,
# which leads to improvements in coverage metrics
# All functions removed in aws_cryptosdk_sign_header
REMOVE_FUNCTION_BODY += EVP_DecryptUpdate
REMOVE_FUNCTION_BODY += EVP_sha256
REMOVE_FUNCTION_BODY += EVP_sha384
REMOVE_FUNCTION_BODY += EVP_sha512

UNWINDSET += array_list_item_generator.0:$(call addone,$(MAX_TABLE_SIZE))
UNWINDSET += aws_cryptosdk_edk_list_elements_are_bounded.0:$(call addone,$(MAX_EDK_LIST_ITEMS))
UNWINDSET += aws_cryptosdk_edk_list_elements_are_valid.0:$(call addone,$(MAX_EDK_LIST_ITEMS))
UNWINDSET += aws_cryptosdk_enc_ctx_serialize.0:$(call addone,$(MAX_TABLE_SIZE))
UNWINDSET += aws_cryptosdk_hdr_size.0:$(call addone,$(MAX_EDK_LIST_ITEMS))
UNWINDSET += aws_cryptosdk_hdr_write.0:$(call addone,$(MAX_EDK_LIST_ITEMS))
UNWINDSET += ensure_cryptosdk_edk_list_has_allocated_list_elements.0:$(call addone,$(MAX_EDK_LIST_ITEMS))
UNWINDSET += memcmp.0:$(call addone,$(MAX_BUFFER_SIZE))

include ../Makefile.common
4 changes: 4 additions & 0 deletions verification/cbmc/proofs/sign_header/cbmc-batch.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
:
This file marks this directory as containing a CBMC proof. This file
is automatically clobbered in CI and replaced with parameters for
running the proof.
99 changes: 99 additions & 0 deletions verification/cbmc/proofs/sign_header/sign_header_harness.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://aws.amazon.com/apache2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

#include <aws/cryptosdk/private/cipher.h>
#include <aws/cryptosdk/private/session.h>
#include <aws/cryptosdk/session.h>
#include <cbmc_invariants.h>
#include <make_common_data_structures.h>
#include <proof_helpers/make_common_data_structures.h>

/**
* A generator function as described in the comment in aws_cryptosdk_hash_elems_array_init_stub.c.
* Also see line 33 of the Makefile.
alex-chew marked this conversation as resolved.
Show resolved Hide resolved
*/
void array_list_item_generator(struct aws_array_list *elems) {
assert(elems->item_size == sizeof(struct aws_hash_element));
for (size_t index = 0; index < elems->length; ++index) {
struct aws_hash_element *val = (struct aws_hash_element *)((uint8_t *)elems->data + (elems->item_size * index));
/* Due to the checks in aws_cryptosdk_enc_ctx_size, no string can have a length > UINT16_MAX. */
struct aws_string *key = ensure_string_is_allocated_nondet_length();
__CPROVER_assume(aws_string_is_valid(key));
/* Due to the cast to uint16, the entire size of the enc_ctx must be less than < UINT16_MAX
* This is a simple way to ensure this without a call to enc_ctx_size. */
__CPROVER_assume(key->len <= UINT8_MAX);
val->key = key;
struct aws_string *value = ensure_string_is_allocated_nondet_length();
__CPROVER_assume(aws_string_is_valid(value));
__CPROVER_assume(value->len <= UINT8_MAX);
val->value = value;
}
}

void sign_header_harness() {
/* Nondet Input */
struct aws_cryptosdk_session *session = malloc(sizeof(*session));

/* Assumptions */
__CPROVER_assume(session != NULL);

enum aws_cryptosdk_alg_id alg_id;
struct aws_cryptosdk_alg_properties *props = aws_cryptosdk_alg_props(alg_id);
__CPROVER_assume(aws_cryptosdk_alg_properties_is_valid(props));
session->alg_props = props;

struct aws_cryptosdk_hdr *hdr = ensure_nondet_hdr_has_allocated_members(MAX_TABLE_SIZE);
__CPROVER_assume(aws_cryptosdk_hdr_members_are_bounded(hdr, MAX_EDK_LIST_ITEMS, MAX_BUFFER_SIZE));
__CPROVER_assume(IMPLIES(hdr != NULL, aws_byte_buf_is_bounded(&hdr->iv, session->alg_props->iv_len)));
__CPROVER_assume(IMPLIES(hdr != NULL, aws_byte_buf_is_bounded(&hdr->auth_tag, session->alg_props->tag_len)));
tegansb marked this conversation as resolved.
Show resolved Hide resolved
ensure_cryptosdk_edk_list_has_allocated_list_elements(&hdr->edk_list);
__CPROVER_assume(aws_cryptosdk_hdr_is_valid(hdr));

__CPROVER_assume(hdr->enc_ctx.p_impl != NULL);
ensure_hash_table_has_valid_destroy_functions(&hdr->enc_ctx);
size_t empty_slot_idx;
__CPROVER_assume(aws_hash_table_has_an_empty_slot(&hdr->enc_ctx, &empty_slot_idx));
alex-chew marked this conversation as resolved.
Show resolved Hide resolved
session->header = *hdr;

struct aws_cryptosdk_sig_ctx *ctx = ensure_nondet_sig_ctx_has_allocated_members();
__CPROVER_assume(aws_cryptosdk_sig_ctx_is_valid_cbmc(ctx));
session->signctx = ctx;

__CPROVER_assume(aws_allocator_is_valid(session->alloc));
__CPROVER_assume(session->state == ST_GEN_KEY);
__CPROVER_assume(session->mode == AWS_CRYPTOSDK_ENCRYPT);

/* Save current state of the data structure */
struct aws_byte_buf old_message_id = session->header.message_id;
struct store_byte_from_buffer old_byte_from_message_id;
save_byte_from_array(session->header.message_id.buffer, session->header.message_id.len, &old_byte_from_message_id);

struct aws_byte_buf old_alg_suite_data = session->header.alg_suite_data;
struct store_byte_from_buffer old_byte_from_alg_suite_data;
save_byte_from_array(
session->header.alg_suite_data.buffer, session->header.alg_suite_data.len, &old_byte_from_alg_suite_data);

struct store_byte_from_buffer old_enc_ctx;
save_byte_from_hash_table(&session->header.enc_ctx, &old_enc_ctx);

/* Operation under verification */
__CPROVER_file_local_session_encrypt_c_sign_header(session);

/* Assertions */
assert(aws_cryptosdk_hdr_is_valid(&session->header));
assert_byte_buf_equivalence(&session->header.message_id, &old_message_id, &old_byte_from_message_id);
assert_byte_buf_equivalence(&session->header.alg_suite_data, &old_alg_suite_data, &old_byte_from_alg_suite_data);
check_hash_table_unchanged(&session->header.enc_ctx, &old_enc_ctx);
}