Skip to content

Commit

Permalink
Add a "binary" option to opt out of UTF8 encoding
Browse files Browse the repository at this point in the history
The SHA256 branch of IsPassword generates binary values to compare,
which may lead to comparing two strings with a different number of
Unicode characters, even when both strings have 26 octets (since UTF8 is
a variable-length encoding). This triggers an error in constant_time_eq
which demands both strings are the same length.

When comparing binary values pass this flag to avoid treating the
inputs as UTF8.
  • Loading branch information
sartak committed Jul 12, 2017
1 parent 74e13ac commit 96075fc
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 5 deletions.
2 changes: 1 addition & 1 deletion lib/RT/User.pm
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,7 @@ sub IsPassword {
my $salt = substr($hash, 0, 4, "");
return 0 unless RT::Util::constant_time_eq(
substr(Digest::SHA::sha256($salt . Digest::MD5::md5(Encode::encode( "UTF-8", $value))), 0, 26),
$hash
$hash, 1
);
} elsif (length $stored == 32) {
# Hex nonsalted-md5
Expand Down
20 changes: 16 additions & 4 deletions lib/RT/Util.pm
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ The two string arguments B<MUST> be of equal length. If the lengths differ,
this function will call C<die()>, as proceeding with execution would create
a timing vulnerability. Length is defined by characters, not bytes.
Strings that should be treated as binary octets rather than Unicode text
should pass a true value for the binary flag.
This code has been tested to do what it claims. Do not change it without
thorough statistical timing analysis to validate the changes.
Expand All @@ -177,7 +180,7 @@ B<https://en.wikipedia.org/wiki/Timing_attack>
=cut

sub constant_time_eq {
my ($a, $b) = @_;
my ($a, $b, $binary) = @_;

my $result = 0;

Expand All @@ -191,9 +194,18 @@ sub constant_time_eq {
my $a_char = substr($a, $i, 1);
my $b_char = substr($b, $i, 1);

# encode() is set to die on malformed
my @a_octets = unpack("C*", encode('UTF-8', $a_char, Encode::FB_CROAK));
my @b_octets = unpack("C*", encode('UTF-8', $b_char, Encode::FB_CROAK));
my (@a_octets, @b_octets);

if ($binary) {
@a_octets = ord($a_char);
@b_octets = ord($b_char);
}
else {
# encode() is set to die on malformed
@a_octets = unpack("C*", encode('UTF-8', $a_char, Encode::FB_CROAK));
@b_octets = unpack("C*", encode('UTF-8', $b_char, Encode::FB_CROAK));
}

die $generic_error if (scalar @a_octets) != (scalar @b_octets);

for (my $j = 0; $j < scalar @a_octets; $j++) {
Expand Down

0 comments on commit 96075fc

Please sign in to comment.