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

t/op/crypt.t fails in FIPS mode #13715

Closed
p5pRT opened this issue Apr 7, 2014 · 6 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Apr 7, 2014

Migrated from rt.perl.org#121591 (status was 'resolved')

Searchable as RT121591$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 7, 2014

From @ppisar

Hello,

if platform implements FIPS mode, then weak algorithms are not available.

DES is one of them and DES is the default algorithm used by glibc's crypt(3).
Then a t/op/crypt.t test fails because perl's crypt() will return undef​:

Use of uninitialized value in substr at ./op/crypt.t line 33.
substr outside of string at ./op/crypt.t line 33.
Use of uninitialized value in substr at ./op/crypt.t line 33.
substr outside of string at ./op/crypt.t line 33.
Use of uninitialized value in string ne at ./op/crypt.t line 33.
Use of uninitialized value in string ne at ./op/crypt.t line 33.
# Failed test 1 - salt makes a difference at ./op/crypt.t line 33
./op/crypt.t ..
Failed 1/4 subtests

Attached patch detects this case and uses a special salt prefix '$5$' to
select SHA-256 which is allowed by FIPS through out all the t/op/crypt.t.

One could just skip the "salt makes a difference" test but I think performing
all the tests with non-undef value is better approach.

-- Petr

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 7, 2014

From @ppisar

0001-t-op-crypt.t-Perform-SHA-256-algorithm-if-default-on.patch
From 8de0fd45cde4826951842f80b6ce109988d47f4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com>
Date: Mon, 7 Apr 2014 12:31:28 +0200
Subject: [PATCH] t/op/crypt.t: Perform SHA-256 algorithm if default one is
 disabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The crypt(3) call may return NULL. This is the case of FIPS-enabled
platforms. Then "salt makes a difference" test would fail.

Signed-off-by: Petr Písař <ppisar@redhat.com>
---
 t/op/crypt.t | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/op/crypt.t b/t/op/crypt.t
index 27c878f..6c43992 100644
--- a/t/op/crypt.t
+++ b/t/op/crypt.t
@@ -28,19 +28,25 @@ BEGIN {
 # bets, given alternative encryption/hashing schemes like MD5,
 # C2 (or higher) security schemes, and non-UNIX platforms.
 
+# Platforms implementing FIPS mode return undef on weak crypto algorithms.
+my $alg = '';       # Use default algorithm
+if ( !defined(crypt("ab", "cd")) ) {
+    $alg = '$5$';   # Use SHA-256
+}
+
 SKIP: {
 	skip ("VOS crypt ignores salt.", 1) if ($^O eq 'vos');
-	ok(substr(crypt("ab", "cd"), 2) ne substr(crypt("ab", "ce"), 2), "salt makes a difference");
+	ok(substr(crypt("ab", $alg . "cd"), 2) ne substr(crypt("ab", $alg. "ce"), 2), "salt makes a difference");
 }
 
 $a = "a\xFF\x{100}";
 
-eval {$b = crypt($a, "cd")};
+eval {$b = crypt($a, $alg . "cd")};
 like($@, qr/Wide character in crypt/, "wide characters ungood");
 
 chop $a; # throw away the wide character
 
-eval {$b = crypt($a, "cd")};
+eval {$b = crypt($a, $alg . "cd")};
 is($@, '',                   "downgrade to eight bit characters");
-is($b, crypt("a\xFF", "cd"), "downgrade results agree");
+is($b, crypt("a\xFF", $alg . "cd"), "downgrade results agree");
 
-- 
1.9.0

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 24, 2014

From @jkeenan

On Mon Apr 07 03​:51​:06 2014, ppisar wrote​:

Hello,

if platform implements FIPS mode, then weak algorithms are not
available.

DES is one of them and DES is the default algorithm used by glibc's
crypt(3).
Then a t/op/crypt.t test fails because perl's crypt() will return
undef​:

Use of uninitialized value in substr at ./op/crypt.t line 33.
substr outside of string at ./op/crypt.t line 33.
Use of uninitialized value in substr at ./op/crypt.t line 33.
substr outside of string at ./op/crypt.t line 33.
Use of uninitialized value in string ne at ./op/crypt.t line 33.
Use of uninitialized value in string ne at ./op/crypt.t line 33.
# Failed test 1 - salt makes a difference at ./op/crypt.t line 33
./op/crypt.t ..
Failed 1/4 subtests

Attached patch detects this case and uses a special salt prefix '$5$'
to
select SHA-256 which is allowed by FIPS through out all the
t/op/crypt.t.

One could just skip the "salt makes a difference" test but I think
performing
all the tests with non-undef value is better approach.

-- Petr

This ticket, which has a patch, was filed in April but has not yet had any discussion.

Could someone familiar with Perl's 'crypt' function take a look?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 24, 2014

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 9, 2014

From @arc

Thanks, applied (with minor wording adjustments to comments and the log message) as d3e5298

--
Aaron Crane ** http​://aaroncrane.co.uk/

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Oct 9, 2014

@arc - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this Oct 9, 2014
@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.