[pacman-dev] [PATCH] signing.c: warn if time went backwards

[Dan McGee]

On Sat, Jun 30, 2012 at 8:16 AM, Florian Pritz <bluewind at xinu.at> wrote:
> GPG signatures have a timestamp which is checked and if it's in the
> future, verification will fail.
>
> Signed-off-by: Florian Pritz <bluewind at xinu.at>
> ---
> Allan suggested to improve the error message printed when verification
> fails, but since I already have this patch I can just as well submit it.
>
>  lib/libalpm/signing.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
> index 1e41716..6148976 100644
> --- a/lib/libalpm/signing.c
> +++ b/lib/libalpm/signing.c
> @@ -20,6 +20,8 @@
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <string.h>
> +#include <time.h>
> +#include <errno.h>
>
>  #if HAVE_LIBGPGME
>  #include <locale.h> /* setlocale() */
> @@ -127,6 +129,10 @@ static int init_gpgme(alpm_handle_t *handle)
>         gpgme_error_t err;
>         gpgme_engine_info_t enginfo;
>
> +       const static char *timestamp_file = "timestamp";
> +       int len;
size_t, and why the blank line between these and the rest of the variables?
> +       char *timestamp_path;
> +
>         if(init) {
>                 /* we already successfully initialized the library */
>                 return 0;
> @@ -143,6 +149,34 @@ static int init_gpgme(alpm_handle_t *handle)
>                                 "pacman-key --init");
>         }
>
> +       len = strlen(sigdir) + strlen(timestamp_file) + 1;
> +       CALLOC(timestamp_path, len, sizeof(char), RET_ERR(handle, ALPM_ERR_MEMORY, -1));
> +       snprintf(timestamp_path, len, "%s%s", sigdir, timestamp_file);

Right now this code only runs as root, but this all breaks down if we
ever add standalone verification capability or anything- chances of
being able to write to sigdir are slim to none.

> +
> +       /* check if time went backwards since last run */
> +       if(!_alpm_access(handle, sigdir, timestamp_file, F_OK)) {
> +               struct stat sb;
> +               time_t t;
> +
> +               if(stat(timestamp_path, &sb) == -1) {
> +                       /* TODO: should we really error here? */
> +                       RET_ERR(handle, ALPM_ERR_SYSTEM, -1);
> +               }
> +
> +               t = time(NULL);
> +               if(t == ((time_t)-1)) {
No need for extra parenthesis here:
if(t == (time_t)-1) {

> +                       _alpm_log(handle, ALPM_LOG_ERROR, _("Failed to determine system time: %s\n"), strerror(errno));
> +               }
I do like being careful about errors, but given that we don't worry
about time() or gettimeofday() failing anywhere else, we probably can
skip this here as well. The reasons it can fail are pretty much not
possible.
> +
> +               if(sb.st_mtime > t) {
> +                       _alpm_log(handle, ALPM_LOG_WARNING, _("Time went backwards, signature verification might fail.\n"));
> +               }
> +       }
> +       /* create file if missing, else update timestamp */
> +       close(creat(timestamp_path, O_RDWR));

DESCRIPTION
     This interface is made obsolete by: open(2).
     The creat() function is the same as:
           open(path, O_CREAT | O_TRUNC | O_WRONLY, mode);

And with all the error checking elsewhere, the chance of open()
failing is about 100x greater than the chance of anything else, and
yet we do no error checking here.

> +
> +       free(timestamp_path);
> +
>         /* calling gpgme_check_version() returns the current version and runs
>          * some internal library setup code */
>         version = gpgme_check_version(NULL);
> --

With all this said, looking at the problem a different way- what
actually happens as far as return codes go from GPGME when the
signature time is in the past? Wouldn't it make sense to just add a
bit of checking in that failure block to print a message if the
returned signature generation time is > current system time, rather
than writing all this stuff to a file?

-Dan