This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC] FIPS compliance and other crypt(3) improvements
> Aha! Ok, then. The description for ENOSYS under crypt still fit, but
> given your comment, I'm now going with EINVAL for this case.
No, ENOSYS does not fit. The wording for that particular function may
not be clear, but elsewhere in the standard it's quite clear that
ENOSYS is for a function that is not available at all--every call will
always fail with ENOSYS. ENOTSUP is specifically for when a function
is available but the arguments to the function request a particular
detail (flag, mode, etc.) that is optional and is not available.
Since in this case it's a feature that is not specified by POSIX, we
are free to use any error code we like except ENOSYS. I think ENOTSUP
is a better match for this case: the argument has a specific meaning
that we understand, but we are not supporting that mode. EINVAL means
it's a malformed argument value, i.e. a bug in the calling program.
Another sensible alternative is EPERM, since it's a system-wide
security policy that is denying access to the functionality.
> /*
> + * Return zero iff C is in the specified alphabet for crypt salt.
> + */
s/zero/false/
> +/*
> * Setup the unit for a new salt
> * Hopefully we'll not see a new salt in each crypt call.
> + * Return FALSE if an unexpected character was found in s[0] or s[1].
> */
s/FALSE/false/
> + int i, n = sizeof (tests) / sizeof (*tests);
Use size_t. Don't declare I here at all. Just use C99 style below.
> + /* Check that crypt won't look at the second character if the first
> + one is invalid. */
> + page = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
> + MAP_PRIVATE | MAP_ANON, -1, 0);
> + if (page == (void*)-1)
Use MAP_FAILED.
> + if (munmap (page + pagesize, pagesize))
> + perror ("munmap");
munmap is not required.
> + if (mmap (page + pagesize, pagesize, 0, MAP_PRIVATE | MAP_ANON,
> + -1, 0) != page + pagesize)
This requires MAP_FIXED. Without it, the address is just a suggestion.
> + perror ("mmap 2");
It should be a hard failure, shouldn't it?
So return here, or use 'error'.
Use test-skeleton.c for the test program.
> for ChangeLog
> 2012-05-15 Alexandre Oliva <aoliva@redhat.com>
Note that we now use date-of-commit in the log lines, so you'll need
to update this when it goes in.
> * sysdeps/unix/sysv/linux/fips-private.h: New.
> * sysdeps/generic/fips-private.h: New, dummy fallback.
"New file", please.
> - result |= strcmp ("$1$saltstri$YMyguxXMBpd2TEZ.vS/3q1", cp);
> +
> + /* MD5 is disabled in FIPS mode. */
> + if (cp)
> + result |= strcmp ("$1$saltstri$YMyguxXMBpd2TEZ.vS/3q1", cp);
>
> return result;
Ideally we'd have a way to test the code regardless of the system
configuration. But I don't see a straightforward way off hand.
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ sysdeps/generic/fips-private.h 2012-05-23 07:48:08.214924392 -0300
> @@ -0,0 +1,12 @@
> +#ifndef _FIPS_PRIVATE_H
> +#define _FIPS_PRIVATE_H
This file is short enough it doesn't really matter to give it a
copyright header. But we tend to do so even for files like this.
And either way, at least give it a descriptive comment.
> +#include <stdbool.h>
> +
> +static inline bool
> +fips_enabled_p (void)
> +{
> + return false;
> +}
As this is the generic file that serves as the model for people
writing new sysdeps files, the comment explaining the contract of
the function should be here.
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ sysdeps/unix/sysv/linux/fips-private.h 2012-05-23 07:51:25.170585521 -0300
> @@ -0,0 +1,51 @@
> +#ifndef _FIPS_PRIVATE_H
> +#define _FIPS_PRIVATE_H
This file needs a copyright header (and don't forget: top line is a
descriptive comment, not the copyright line).
> +{
> + static int checked;
There's no reason for arcane and unexplained logic with magic int
values here. Just make it an enum.
> + if (!checked)
> + checked = -2;
This needs a comment as it is, but would be self-explanatory if it
were an enum.
Thanks,
Roland