[pacman-dev] [PATCH] signing.c: warn if time went backwards
GPG signatures have a timestamp which is checked and if it's in the future, verification will fail. Signed-off-by: Florian Pritz <bluewind@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; + 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); + + /* 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)) { + _alpm_log(handle, ALPM_LOG_ERROR, _("Failed to determine system time: %s\n"), strerror(errno)); + } + + 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)); + + free(timestamp_path); + /* calling gpgme_check_version() returns the current version and runs * some internal library setup code */ version = gpgme_check_version(NULL); -- 1.7.11.1
On Sat, Jun 30, 2012 at 8:16 AM, Florian Pritz <bluewind@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@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
participants (2)
-
Dan McGee
-
Florian Pritz