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

poly1305 internals: Remove unneeded I-U-F buffering. #2270

Closed
wants to merge 1 commit into from
Closed
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
poly1305 internals: Remove unneeded I-U-F buffering.
`CRYPTO_poly1305_update` was designed to buffer any partial blocks,
only passing full blocks to poly1305_update. Then
`CRYPTO_poly1305_finish` would pass in the final partial block.

`chacha20_poly1305`'s poly1305_update_padded_16 was working around that
logic to ensure that a partial block never got buffered. Then it was
doing extra work to pad the last block with zeros.

These two mechanisms were basically cancelling each other out. Instead,
just avoid all the work.

This removes some non-trivial buffer management from C.
  • Loading branch information
briansmith committed Jan 22, 2025
commit 0b1967e6895323d153a43d7eef1d400bba32de48
2 changes: 1 addition & 1 deletion build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -857,7 +857,7 @@ fn prefix_all_symbols(pp: char, prefix_prefix: &str, prefix: &str) -> String {
"CRYPTO_poly1305_finish_neon",
"CRYPTO_poly1305_init",
"CRYPTO_poly1305_init_neon",
"CRYPTO_poly1305_update",
"CRYPTO_poly1305_update_padded_16",
"CRYPTO_poly1305_update_neon",
"ChaCha20_ctr32",
"ChaCha20_ctr32_avx2",
Expand Down
52 changes: 6 additions & 46 deletions crypto/poly1305/poly1305.c
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#include <ring-core/poly1305.h>

#include "../internal.h"

#include "ring-core/check.h"

#if defined(__GNUC__) || defined(__clang__)
#pragma GCC diagnostic ignored "-Wsign-conversion"
Expand All @@ -32,8 +32,6 @@ struct poly1305_state_st {
uint32_t r0, r1, r2, r3, r4;
uint32_t s1, s2, s3, s4;
uint32_t h0, h1, h2, h3, h4;
uint8_t buf[16];
size_t buf_used;
uint8_t key[16];
};

Expand Down Expand Up @@ -179,61 +177,23 @@ void CRYPTO_poly1305_init(poly1305_state *statep, const uint8_t key[32]) {
state->h3 = 0;
state->h4 = 0;

state->buf_used = 0;
OPENSSL_memcpy(state->key, key + 16, sizeof(state->key));
}

void CRYPTO_poly1305_update(poly1305_state *statep, const uint8_t *in,
size_t in_len) {
struct poly1305_state_st *state = poly1305_aligned_state(statep);

// Work around a C language bug. See https://crbug.com/1019588.
if (in_len == 0) {
return;
}

if (state->buf_used) {
size_t todo = 16 - state->buf_used;
if (todo > in_len) {
todo = in_len;
}
for (size_t i = 0; i < todo; i++) {
state->buf[state->buf_used + i] = in[i];
}
state->buf_used += todo;
in_len -= todo;
in += todo;

if (state->buf_used == 16) {
poly1305_update(state, state->buf, 16);
state->buf_used = 0;
}
}
void CRYPTO_poly1305_update_padded_16(
poly1305_state *statep, const uint8_t *in, size_t in_len) {
debug_assert_nonsecret(in_len > 0);

if (in_len >= 16) {
size_t todo = in_len & ~0xf;
poly1305_update(state, in, todo);
in += todo;
in_len &= 0xf;
}
struct poly1305_state_st *state = poly1305_aligned_state(statep);

if (in_len) {
for (size_t i = 0; i < in_len; i++) {
state->buf[i] = in[i];
}
state->buf_used = in_len;
}
poly1305_update(state, in, in_len);
}

void CRYPTO_poly1305_finish(poly1305_state *statep, uint8_t mac[16]) {
struct poly1305_state_st *state = poly1305_aligned_state(statep);
uint32_t g0, g1, g2, g3, g4;
uint32_t b, nb;

if (state->buf_used) {
poly1305_update(state, state->buf, state->buf_used);
}

b = state->h0 >> 26;
state->h0 = state->h0 & 0x3ffffff;
state->h1 += b;
Expand Down
22 changes: 5 additions & 17 deletions src/aead/chacha20_poly1305/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@
let (counter, poly1305_key) = begin(chacha20_key, nonce, aad, in_out, cpu)?;
let mut auth = poly1305::Context::from_key(poly1305_key, cpu);

poly1305_update_padded_16(&mut auth, aad.as_ref());
auth.update_padded_16(aad.as_ref());

Check warning on line 79 in src/aead/chacha20_poly1305/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/aead/chacha20_poly1305/mod.rs#L79

Added line #L79 was not covered by tests
chacha20_key.encrypt(counter, in_out.into(), cpu);
poly1305_update_padded_16(&mut auth, in_out);
auth.update_padded_16(in_out);

Check warning on line 81 in src/aead/chacha20_poly1305/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/aead/chacha20_poly1305/mod.rs#L81

Added line #L81 was not covered by tests
Ok(finish(auth, aad.as_ref().len(), in_out.len()))
}

Expand Down Expand Up @@ -110,8 +110,8 @@
let (counter, poly1305_key) = begin(chacha20_key, nonce, aad, in_out.input(), cpu)?;
let mut auth = poly1305::Context::from_key(poly1305_key, cpu);

poly1305_update_padded_16(&mut auth, aad.as_ref());
poly1305_update_padded_16(&mut auth, in_out.input());
auth.update_padded_16(aad.as_ref());
auth.update_padded_16(in_out.input());

Check warning on line 114 in src/aead/chacha20_poly1305/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/aead/chacha20_poly1305/mod.rs#L113-L114

Added lines #L113 - L114 were not covered by tests
let in_out_len = in_out.len();
chacha20_key.encrypt(counter, in_out, cpu);
Ok(finish(auth, aad.as_ref().len(), in_out_len))
Expand Down Expand Up @@ -152,18 +152,6 @@
let (alen, clen) = block.split_at_mut(poly1305::BLOCK_LEN / 2);
alen.copy_from_slice(&u64::to_le_bytes(u64_from_usize(aad_len)));
clen.copy_from_slice(&u64::to_le_bytes(u64_from_usize(in_out_len)));
auth.update(&block);
auth.update_padded_16(&block);

Check warning on line 155 in src/aead/chacha20_poly1305/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/aead/chacha20_poly1305/mod.rs#L155

Added line #L155 was not covered by tests
auth.finish()
}

#[inline]
fn poly1305_update_padded_16(ctx: &mut poly1305::Context, input: &[u8]) {
if !input.is_empty() {
ctx.update(input);
let remainder_len = input.len() % poly1305::BLOCK_LEN;
if remainder_len != 0 {
const ZEROES: [u8; poly1305::BLOCK_LEN] = [0; poly1305::BLOCK_LEN];
ctx.update(&ZEROES[..(poly1305::BLOCK_LEN - remainder_len)])
}
}
}
17 changes: 10 additions & 7 deletions src/aead/poly1305.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

use super::{Tag, TAG_LEN};
use crate::{c, cpu};
use core::num::NonZeroUsize;

/// A Poly1305 key.
pub(super) struct Key {
Expand Down Expand Up @@ -91,12 +92,14 @@ impl Context {
}

#[inline(always)]
pub fn update(&mut self, input: &[u8]) {
dispatch!(
self.cpu_features =>
(CRYPTO_poly1305_update | CRYPTO_poly1305_update_neon)
(statep: &mut poly1305_state, input: *const u8, in_len: c::size_t)
(&mut self.state, input.as_ptr(), input.len()));
pub fn update_padded_16(&mut self, input: &[u8]) {
if let Some(len) = NonZeroUsize::new(input.len()) {
dispatch!(
self.cpu_features =>
(CRYPTO_poly1305_update_padded_16 | CRYPTO_poly1305_update_neon)
(statep: &mut poly1305_state, input: *const u8, in_len: c::NonZero_size_t)
(&mut self.state, input.as_ptr(), len));
}
}

pub(super) fn finish(mut self) -> Tag {
Expand All @@ -116,7 +119,7 @@ impl Context {
/// poly1305 test vectors.
pub(super) fn sign(key: Key, input: &[u8], cpu_features: cpu::Features) -> Tag {
let mut ctx = Context::from_key(key, cpu_features);
ctx.update(input);
ctx.update_padded_16(input);
ctx.finish()
}

Expand Down
Loading