search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.82k stars 4.62k forks source link

System.Security.Cryptography.AesGcm - decryption allows truncated tags #71366

Closed sdrapkin closed 1 year ago

sdrapkin commented 2 years ago

Edited by @vcsjones

API Proposal:

As the issue outlines below, this proposes changes to AesGcm because it's current design makes it prone to misuse of allowing truncated authentication tags during decryption.

This proposes new constructors that accept the authentication tag size. During decryption or encryption, the tag parameter must match this specified exactly. This means the caller must decide at instantiation what tag size is correct, not what callers of Encrypt or Decrypt are provided.

The current constructors will continue to have the same existing behavior. However, since this is an issue around misuse, this also proposes obsoleting the existing constructors to steer people toward using constructors which take the authentication tag size.

namespace System.Security.Cryptography;

public partial class AesGcm {

+  public AesGcm(byte[] key, int tagSizeInBytes);
+  public AesGcm(ReadOnlySpan<byte> key, int tagSizeInBytes);

+  [Obsolete("AesGcm should indicate the required tag size for encryption and decryption. Use a constructor that accepts the tag size.", DiagnosticId = "SYSLIB9999")]
   public AesGcm(byte[] key);
+  [Obsolete("AesGcm should indicate the required tag size for encryption and decryption. Use a constructor that accepts the tag size.", DiagnosticId = "SYSLIB9999")]
   public AesGcm(ReadOnlySpan<byte> key);

+  public int? TagSizeInBytes { get; }
}

Original issue:

void Main()
{
    Span<byte> plaintext = Encoding.UTF8.GetBytes("[Hello World!]");
    Span<byte> key = new byte[32];
    Span<byte> nonce = new byte[12];
    Span<byte> ciphertext = new byte[plaintext.Length];
    Span<byte> tag = new byte[16];

    using var gcm = new AesGcm(key);

    Encoding.UTF8.GetString(plaintext).Dump();

    gcm.Encrypt(nonce, plaintext, ciphertext, tag);
    plaintext.Clear();
    gcm.Decrypt(nonce, ciphertext, tag.Slice(0, 12), plaintext);
    Encoding.UTF8.GetString(plaintext).Dump();
}

Scenario: A naive (documentation-reading) user uses AesGcm to encrypt plaintext and produce ciphertext with the maximum allowed tag of 16 bytes (ie. naively expecting ~128 bits of tag strength). However the Decrypt API is quite happy to accept truncated 12-byte tags, which means that the attacker can truncate all tags to 12 bytes for ~96 bits of tag strength (assuming decryption oracle is readily available).

Would that suggest that AesGcm tags as implemented in .NET realistically provide close to 96-32=64 bits of security? If yes - is anyone concerned about that? (both in terms of implementation/security and in terms of documentation). If no - please explain and help me understand what I'm missing. Thanks.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones See info in area-owners.md if you want to be subscribed.

Issue Details
```csharp void Main() { Span plaintext = Encoding.UTF8.GetBytes("[Hello World!]"); Span key = new byte[32]; Span nonce = new byte[12]; Span ciphertext = new byte[plaintext.Length]; Span tag = new byte[16]; using var gcm = new AesGcm(key); Encoding.UTF8.GetString(plaintext).Dump(); gcm.Encrypt(nonce, plaintext, ciphertext, tag); plaintext.Clear(); gcm.Decrypt(nonce, ciphertext, tag.Slice(0, 12), plaintext); Encoding.UTF8.GetString(plaintext).Dump(); } ``` Scenario: A naive (documentation-reading) user uses `AesGcm` to encrypt plaintext and produce ciphertext with the maximum allowed tag of 16 bytes (ie. naively expecting ~128 bits of tag strength). However the `Decrypt` API is quite happy to accept truncated 12-byte tags, which means that the attacker can truncate all tags to 12 bytes for ~96 bits of tag strength (assuming decryption oracle is readily available). * `AesGcm` `n`-bit tags provide only `n-k` bits of authentication security for messages that are `2^k` blocks long * Large files at ~64GiB size are quite common, and it's plausible to assume that close to 2^32 blocks are routinely encrypted Would that suggest that `AesGcm` tags as implemented in .NET realistically provide close to `96-32=64` bits of security? If yes - is anyone concerned about that? (both in terms of implementation/security and in terms of documentation). If no - please explain and help me understand what I'm missing. Thanks.
Author: sdrapkin
Assignees: -
Labels: `area-System.Security`
Milestone: -
vcsjones commented 2 years ago

Tag truncation is just how AES GCM supports short tags, as demonstrated by:

using System.Security.Cryptography;

AesGcm aes = new AesGcm(new byte[128 / 8]);
byte[] nonce = new byte[96 / 8];

byte[] tag16 = new byte[16];
aes.Encrypt(nonce, new byte[32], new byte[32], tag16);

byte[] tag12 = new byte[12];
aes.Encrypt(nonce, new byte[32], new byte[32], tag12);

Console.WriteLine(Convert.ToHexString(tag16));
Console.WriteLine(Convert.ToHexString(tag12));

// Prints:
// 40490AF4805606B2A3A2E793E3500066
// 40490AF4805606B2A3A2E793

This is not a .NET-ism, either. For example, Ruby:

require 'openssl'

cipher = OpenSSL::Cipher.new('aes-128-gcm').encrypt
cipher.key = "\0" * 16
cipher.iv = "\0" * 12
cipher.auth_data = ""

encrypted = cipher.update('a' * 32) + cipher.final
tag16 = cipher.auth_tag

cipher = OpenSSL::Cipher.new('aes-128-gcm').decrypt
cipher.key = "\0" * 16
cipher.iv = "\0" * 12
tag12 = tag16.slice(0, 12)
cipher.auth_tag = tag12
decrypted = cipher.update(encrypted) + cipher.final

puts decrypted

Ruby will even let you truncate it below 12 octets.

Since the AES-GCM implementation supports short tags, Decrypt can't tell the difference between a tag that was truncated legitimately or illegitimately.

Consumers of AesGcm should ensure that the tag length that is being supplied is the expected length.

sdrapkin commented 2 years ago

Since the AES-GCM implementation supports short tags, Decrypt can't tell the difference between a tag that was truncated legitimately or illegitimately. Consumers of AesGcm should ensure that the tag length that is being supplied is the expected length.

Thanks for confirming that my understanding is correct.

I see 2 issues:

  1. Insufficient documentation, missing guidance on what AesGcm consumers are expected to ensure for tag lengths. The description for the tag parameter to .Decrypt states: The authentication tag produced for this message during encryption (which is clearly incorrect when truncated tags are accepted, since they were likely never produced by the Encryption).
  2. Let's assume documentation is updated (ex. the above quote is added to it). How are developers supposed to comply?
    • option-1: validate a fixed-tag-of-length-N expectation. This is practical, but already makes Decrypt unsafe for direct exposure (ie. requires a tag-length validator in front of it). N must become part of the caller-state/protocol, since it is not part of the Encrypt output.
    • option-2: variable-length-tags. This would be very challenging/unsafe/insecure to implement with AesGcm primitive API alone. Our key assumption is that ciphertext and tag are malleable (which is precisely what the tag is supposed to guard against). While the tag-length could be made non-malleable (option-1), ciphertext-length cannot be made non-malleable. While Microsoft (rightfully) does not prescribe how the tag and ciphertext are to be stored, in the vast majority of uses they are concatenated into one bytestream (order is not relevant). Thus, it is not possible to calculate the correct value of tag-length N from the (ciphertext+tag) bytestream. One approach is to call Decrypt 5 times for each of the 5 possible values of N (not an appealing idea for obvious reasons).

Given that it seems option-1 is the only practical/reasonable choice, it would also need to be added to AesGcm documentation (ie. guidance that N must be explicitly fixed for safe operation of Decrypt, which includes tag validation).

Finally, it might be a good idea to also document a caution that AesGcm tag of length N bytes should not be assumed to provide N*8 bits of authentication strength in all scenarios.

Thoughts?

sdrapkin commented 2 years ago

What GCM specification does S.S.C.AesGcm actually implement?

sdrapkin commented 2 years ago

Proposed improvements to AesGcm APIs (non-breaking, backwards-compatible):

public class AesGcm
{
    // existing ctors
    public AesGcm(ReadOnlySpan<byte> key) { }
    public AesGcm(byte[] key) { }

    // new field
    readonly int _fixedTagLength = 0;

    // new ctor
    public AesGcm(ReadOnlySpan<byte> key, int fixedTagLength)
    {
        _fixedTagLength = fixedTagLength; // also do [12-16] range check
    }

    // new ctor
    public AesGcm(byte[] key, int fixedTagLength)
    {
        _fixedTagLength = fixedTagLength; // also do [12-16] range check
    }

Change internal implementation for

static void CheckParameters(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> nonce, ReadOnlySpan<byte> tag)

by adding the following check to it:

int fixedTagLength = _fixedTagLength;
if (fixedTagLength != 0 && fixedTagLength != tag.Length)
{
    throw new ArgumentException(System.SR.Cryptography_InvalidTagLength, "tag");
}
sdrapkin commented 2 years ago

Example of a likely vulnerability in Microsoft's code due to incorrect assumptions of what AesGcm.Decrypt does/doesn't do:

MS domain: AzureAD Repo: azure-activedirectory-identity-model-extensions-for-dotnet Code in question

If I traced their call-paths right, the authenticationTag is coming straight from JWT token field, which is treated as any-length base64-encoded string. An attacker can be truncating all JWT tokens to 12-byte Auth-tags which reduces GCM tag strength to 96 bits of security at most. I doubt AzureAD team realizes this...

I'm sure there are many more cases like this one, where the authentication Tag lives separately from ciphertext (ie. not a mere take X fixed bytes from the end of ciphertext slicing).

AesGcm not having safe-by-default constructors (that default to fixed tag of 16 bytes) is going to be the cause of numerous security vulnerabilities. In the meantime I've created AesGcmStrict wrapper to help deal with this.

bartonjs commented 2 years ago

We're exposing GCM in the same way the underlying providers are, which leaves the algorithms loose tag processing as an application problem. (I'd've included the tag length in the tag calculation, but I guess the GCM designers disagree)

Generally the application context should know what the tag size is. In the case of JWA, it's supposed to always be 128 bits (https://datatracker.ietf.org/doc/html/rfc7518#section-4.7), which is up to the implementer to verify.

Adding ctor overloads for the expected tag size is reasonable, and not limiting to anyone following the SP800-38D recommendation, and I guess helps provide a standardized exception message to anyone who chooses to utilize it.

sdrapkin commented 2 years ago

Thanks for the response. I would encourage .NET team to consider adding tag-length-fixing constructor overloads to AesGcm, based on @bartonjs reply.

Do note that the AesCcm behavior does not have the same problem - ie. truncated AesCcm tags (that are still length-permitted based on AesCcm.TagByteSizes) will fail to decrypt. This creates 2 different (and undocumented) behaviors for AesGcm and AesCcm APIs in .NET.

Just do a simple sanity test: ask any of your esteemed cryptography-aware colleagues the following simple question:

I've got an AesGcm-encrypted ciphertext, and I'm trying to decrypt it with a truncated tag - will it work?

..and watch how many no's you're going to get.

I believe it would be somewhat irresponsible to take the position that "loose AesGcm tag processing" is application's problem. Arguing that .NET team is merely exposing the deficiencies of underlying GCM providers is a dodge at best (at the very least you cannot vouch for all possible GCM providers on all supported current and future .NET-running platforms). However let me provide a specification-based argument:

NIST SP 800-38D :

What this means is that for a specific instance of AesGcm implemented to the spec:

var one_gcm = new AesGcm(key);

...one_gcm.Decrypt(...) MUST NOT accept tag-lengths that differ from those produced by one_gcm.Encrypt(...)

Ask any FIPS auditor/validator lab, and they should tell you that the GCM spec wording above is specifically intended to prevent tag truncations under a fixed key. The sister CCM spec (800-38C) which targets the same AEAD behavior has the same constraint:

Thus it is quite likely that the current status of AesGcm in .NET is that it is not spec-compliant.

vcsjones commented 2 years ago

Thus it is quite likely that the current status of AesGcm in .NET is that it is not spec-compliant.

I'm not quite sure I follow this logic.

Given the selection of an approved block cipher, key, and an associated tag length, the inputs to

The "selected" tag length just happens to be whatever tag.Length is.

CCM does not have this problem because CCM bakes the MAC length in to the control information block:

The leading octet of the first block of the formatting, B0, contains four flags for control information: two single bits, called Reserved and Adata, and two strings of three bits, to encode the values t and q.

Where t is "The octet length of the MAC." CCM is an entirely different mode. While CCM and GCM are both AEADs I do not think it is entirely reasonable to consider their behaviors and design as interchangeable. GCM has no control information block.

By your logic then, is OpenSSL not "spec compliant" for GCM?

//tl;dr: OpenSSL encrypting with a 16 byte tag and decrypting with a 12 byte tag.

#include <assert.h>
#include <stdio.h>
#include <string.h>
#include <openssl/evp.h>

void dump(unsigned char* data, int len)
{
    for (int i = 0; i < len; i++) {
        printf("%02x", data[i]);
    }
    putchar('\n');
}

int main(int argc, char *argv[])
{
    EVP_CIPHER_CTX *ctx = EVP_CIPHER_CTX_new();

    unsigned char key[16] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 };
    unsigned char nonce[12] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12 };
    unsigned char plaintext[6] = "hello";
    unsigned char decrypted[6] =  { 0 };
    unsigned char ciphertext[6] = { 0 };
    unsigned char tag[16] = { 0 };

    if (EVP_EncryptInit_ex(ctx, EVP_aes_128_gcm(), NULL, NULL, NULL) != 1) {
        printf("Could not initialize");
        abort();
    }

    if (EVP_EncryptInit_ex(ctx, NULL, NULL, key, nonce) != 1) {
        printf("Could not set key or nonce");
        abort();
    }

    int len = 0, ciphertextLength = 0;

    if (EVP_EncryptUpdate(ctx, ciphertext, &len, plaintext, 6) != 1) {
        printf("Encrypt failed.");
        abort();
    }

    ciphertextLength += len;

    if (EVP_EncryptFinal_ex(ctx, ciphertext + len, &len) != 1) {
        printf("Final failed.");
        abort();
    }

    ciphertextLength += len;

    if (ciphertextLength != 6) {
        printf("6 bytes in should be 6 bytes out.");
        abort();
    }

    printf("Ciphertext: ");
    dump(ciphertext, ciphertextLength);

    if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, 16, tag) != 1) {
        printf("Could not get the tag.");
        abort();
    }

    printf("Tag: ");
    dump(tag, sizeof tag);

    unsigned char truncatedTag[12] = { 0 };
    memcpy(truncatedTag, tag, sizeof truncatedTag);

    printf("Truncated Tag: ");
    dump(truncatedTag, sizeof truncatedTag);

    EVP_CIPHER_CTX_free(ctx);

    ctx = EVP_CIPHER_CTX_new();

    if (EVP_DecryptInit_ex(ctx, EVP_aes_128_gcm(), NULL, NULL, NULL) != 1) {
        printf("Could not initialize");
        abort();
    }

    if (EVP_DecryptInit_ex(ctx, NULL, NULL, key, nonce) != 1) {
        printf("Could not set key or nonce");
        abort();
    }

    assert(sizeof truncatedTag == 12);

    if(!EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, sizeof truncatedTag, truncatedTag)) {
        printf("Could not set truncated authentication tag.");
        abort();
    }

    int plaintextLength = 0;

    if (EVP_DecryptUpdate(ctx, decrypted, &len, ciphertext, ciphertextLength) != 1) {
        printf("Decrypt failed.");
        abort();
    }

    plaintextLength += len;

    int result = EVP_DecryptFinal_ex(ctx, plaintext + plaintextLength, &len);

    if (result > 0) {
        printf("Decrypted successfully");
        plaintextLength += len;
    }
    else {
        printf("Decryption failed.");
    }
}
sdrapkin commented 2 years ago

@vcsjones In your C/OpenSSL code you create one GCM context to encrypt (which produces 16-byte tag), then you free it, then you create another new GCM context to decrypt, then you use that 2nd context to successfully decrypt with a truncated 12-byte tag.

This is not the correct comparison. I'm not very familiar with low-level OpenSSL APIs - perhaps OpenSSL cannot use the same GCM context to encrypt and decrypt, and thus requires a new context for each operation. Your Ruby example (which seems to wrap OpenSSL) also uses 2 different GCM contexts to encrypt and decrypt.

The GCM spec is quite clear (Section-7 "GCM specification") that tag-length is a "prerequisite" (prerequisites are fixed across many invocations of Encrypt or Decrypt function), and must be associated with the key. The GCM decryption function specification (Section 7.2) is explicit in saying that one of its prerequisites is a "supported tag length t associated with the key".

OpenSSL low-level functions are a toolkit for "build-your-own-GCM" (here's AES, here's CTR-mode, here's GMAC, and here's XOR - go have fun).

This is how Golang does it. This should have as much relevance to .NET as how OpenSSL does it (ie. not relevant, but at least Golang does it correctly, by binding tag-size to GCM context and having a default of tag-size=16).

bartonjs commented 2 years ago

@sdrapkin Your insistence that our implementation is wrong and/or against spec isn't productive.

We have the implementation we have, and we're not going to change it. The specific example that you cited from JWA nearly automatically requires separate contexts that are only tied together by a specification; and NIST often writes "this is how things are" which only means "this is the only acceptable use for US-Gov and impacted parties" (such as that a single RSA key should be PKCS1 xor PSS, or signing xor encryption... but our underlying providers have the same loosy-goosey model we have (which is what informed ours).

The usability concern behind "let me tell you what tag size I intend to use at construction time to save me the redundancy of checking tag.Length in my code" is valid. So some new API is justified to enhance that scenario.

The cryptography stack in .NET is also mostly intended as low-level primitives. It's a key. Use it however you want. We don't block duplicated nonces... that's an application problem. But many specs will tell you they MUST be unique. At some point things just become a caller/application problem.

sdrapkin commented 2 years ago

@bartonjs I understand. Thanks for reviewing this issue. Let's do whatever is productive.

owlstead commented 2 years ago

Security check such as tag size checks should not be left to user of libraries. The fact that the possibility of tag size truncation is not even mentioned in the API's clearly shows a lack of responsibility by the creators the API. API's should contain indication on how to use them correctly and securely. Just brushing this off as something that OpenSSL does as well is dishonest - creators or API's are encouraged to think on their own and know their field. Given the track record of OpenSSL implementation and documentation it is not even a good argument by itself.

Note that the Java implementation defaults to a 128 bit tag size, and that you need to configure the tag size explicitly using GcmParameterSpec. So there are certainly implementations that handle this better. Of course, since the tag size is not mentioned explicitly in the list of Java supported algorithms, other providers within that eco-system could choose otherwise. There is also the issue that tag verification is not explicit in the Java API. The Java API is far from perfect, but it does show that other libraries do validate the complete tag.

That all said, I think adding the constructors with an explicit tag size is a necessity to create a secure API. Given that the current constructors do not indicate a tag size I would strongly suggest to both deprecate these constructors and provide a clear warning in the documentation. It might be worthwhile to think of a way (factory method?) that defaults to a 128 bit tag size. If the current constructors are not deprecated then devs will simply choose the easier, insecure option with fewer parameters.

I do agree with bartonjs that it does make sense to think in solutions; the API cannot be changed in such a way that backwards compatibility is harmed. It may be that the current API is used for e.g. 12 byte tags, where the tag size is validated.

vcsjones commented 1 year ago

The top post has been updated with an API proposal.

bartonjs commented 1 year ago

Video

Looks good as proposed.


namespace System.Security.Cryptography;

public partial class AesGcm {

+  public AesGcm(byte[] key, int tagSizeInBytes);
+  public AesGcm(ReadOnlySpan<byte> key, int tagSizeInBytes);

+  [Obsolete("AesGcm should indicate the required tag size for encryption and decryption. Use a constructor that accepts the tag size.", DiagnosticId = "SYSLIB9999")]
   public AesGcm(byte[] key);
+  [Obsolete("AesGcm should indicate the required tag size for encryption and decryption. Use a constructor that accepts the tag size.", DiagnosticId = "SYSLIB9999")]
   public AesGcm(ReadOnlySpan<byte> key);

+  public int? TagSizeInBytes { get; }
}
ghost commented 1 year ago

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

  1. [x] Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

vcsjones commented 1 year ago

Breaking change doc: https://github.com/dotnet/docs/issues/35338