From fbfc75d2a7ea566f7499addaa3316b7d5ca470a9 Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 4 Feb 2025 08:47:13 +0000 Subject: [PATCH 1/2] Fix spec compliant record size verification Spec compliant records were throwing issues during decryption due to a bug in record size verification --- .../google/crypto/tink/apps/webpush/WebPushConstants.java | 7 +++++++ .../crypto/tink/apps/webpush/WebPushHybridDecrypt.java | 7 +++++-- .../crypto/tink/apps/webpush/WebPushHybridEncrypt.java | 5 +++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushConstants.java b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushConstants.java index c864021..aa7218b 100644 --- a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushConstants.java +++ b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushConstants.java @@ -63,6 +63,13 @@ final class WebPushConstants { static final int CIPHERTEXT_OVERHEAD = CONTENT_CODING_HEADER_SIZE + PADDING_DELIMETER_SIZE + DEFAULT_PADDING_SIZE + TAG_SIZE; + // https://www.rfc-editor.org/rfc/rfc8188#section-2.1 + // > rs: Values smaller than 18 are invalid. + // + // * min data length: 1 + // * padding delimeter: 1 + // * AES-GCM tag size: 16 + static final int MIN_RS = 18; static final int MAX_CIPHERTEXT_SIZE = 4096; static final String HMAC_SHA256 = "HMACSHA256"; diff --git a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java index 9de4a66..2f0179f 100644 --- a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java +++ b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java @@ -111,7 +111,10 @@ private WebPushHybridDecrypt(Builder builder) throws GeneralSecurityException { } this.authSecret = builder.authSecret; - if (builder.recordSize < WebPushConstants.CIPHERTEXT_OVERHEAD + // Nothing prevent a max bound for rs. But we know that push messages + // are at most MAX_CIPHERTEXT_SIZE (0x1000), and it is usually set to + // this value for convenience. + if (builder.recordSize < WebPushConstants.MIN_RS || builder.recordSize > WebPushConstants.MAX_CIPHERTEXT_SIZE) { throw new IllegalArgumentException( String.format( @@ -229,7 +232,7 @@ public byte[] decrypt(final byte[] ciphertext, final byte[] contextInfo /* unuse int recordSize = record.getInt(); if (recordSize != this.recordSize - || recordSize < ciphertext.length + || recordSize < (ciphertext.length - WebPushConstants.CONTENT_CODING_HEADER_SIZE) || recordSize > WebPushConstants.MAX_CIPHERTEXT_SIZE) { throw new GeneralSecurityException("invalid record size: " + recordSize); } diff --git a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridEncrypt.java b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridEncrypt.java index 4d1d5fb..7b7e3cd 100644 --- a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridEncrypt.java +++ b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridEncrypt.java @@ -134,7 +134,7 @@ public Builder() {} */ @CanIgnoreReturnValue public Builder withRecordSize(int val) { - if (val < WebPushConstants.CIPHERTEXT_OVERHEAD + if (val < WebPushConstants.MIN_RS || val > WebPushConstants.MAX_CIPHERTEXT_SIZE) { throw new IllegalArgumentException( String.format( @@ -216,7 +216,8 @@ public byte[] encrypt(final byte[] plaintext, final byte[] contextInfo /* unused throw new GeneralSecurityException("contextInfo must be null because it is unused"); } - if (plaintext.length > recordSize - paddingSize - WebPushConstants.CIPHERTEXT_OVERHEAD) { + // rs >= len(plaintext + AES-GCM tag + padding delimiter + padding) + if (plaintext.length > recordSize - paddingSize - WebPushConstants.TAG_SIZE - 1) { throw new GeneralSecurityException( String.format( "plaintext too long; with record size = %d and padding size = %d, plaintext cannot" From a424e1d3305ea666f97a5bd528d4a569a48e03fa Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 4 Feb 2025 09:55:42 +0000 Subject: [PATCH 2/2] Fix decryption fail with non-constant recordSize The recordSize is get from the encrypted payload. If we check against the exact value of this.recordSize, and the server sends non-constant RS values, then we need to parse the message twice: once to get the RS for the builder, and again during decryption. As of RFC8818, recordSize is used to set a maximum bound for the encrypted payload to parse. Therefore, there is no issues if the recordSize we receive is lower than the one we defined. As long as this recordSize is higher than the actual recode size. this.recordSize is always < MAX_CIPHERTEXT_SIZE. --- .../crypto/tink/apps/webpush/WebPushHybridDecrypt.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java index 2f0179f..fb264c5 100644 --- a/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java +++ b/webpush/src/main/java/com/google/crypto/tink/apps/webpush/WebPushHybridDecrypt.java @@ -231,9 +231,10 @@ public byte[] decrypt(final byte[] ciphertext, final byte[] contextInfo /* unuse record.get(salt); int recordSize = record.getInt(); - if (recordSize != this.recordSize - || recordSize < (ciphertext.length - WebPushConstants.CONTENT_CODING_HEADER_SIZE) - || recordSize > WebPushConstants.MAX_CIPHERTEXT_SIZE) { + // Fail if the recordSize is too low for the ciphertext, + // or higher than the recordSize we defined + if (recordSize > this.recordSize + || recordSize < (ciphertext.length - WebPushConstants.CONTENT_CODING_HEADER_SIZE)) { throw new GeneralSecurityException("invalid record size: " + recordSize); }