Bug 194723 - [patch] update for variable time windows when using security/pam_google_authenticator for totp authn
Summary: [patch] update for variable time windows when using security/pam_google_authe...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-ports-bugs (Nobody)
URL:
Keywords: patch-ready
Depends on:
Blocks:
 
Reported: 2014-10-31 20:41 UTC by paul
Modified: 2014-11-22 18:35 UTC (History)
2 users (show)

See Also:


Attachments
Unified diff for security/pam_google_authenticator/Makefile (756 bytes, patch)
2014-11-01 01:17 UTC, paul
no flags Details | Diff
Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c (2.32 KB, patch)
2014-11-01 01:18 UTC, paul
no flags Details | Diff
(UPDATED) Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c (1.81 KB, patch)
2014-11-03 21:22 UTC, paul
no flags Details | Diff
(UPDATE, again) Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c (2.02 KB, patch)
2014-11-03 21:29 UTC, paul
no flags Details | Diff
Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c (1.82 KB, patch)
2014-11-04 17:16 UTC, paul
no flags Details | Diff
Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c (2.03 KB, patch)
2014-11-04 17:18 UTC, paul
no flags Details | Diff
Unified diff for security/pam_google_authenticator/Makefile (723 bytes, patch)
2014-11-18 21:03 UTC, paul
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description paul 2014-10-31 20:41:58 UTC
I have need to use the pam_google_authenticator.so to authenticate users who have been issued TOTP hardware tokens.  Unfortunately, the hardware tokens use a 60 second period and pam_google_authenticator is hardcoded for 30 second periods.  Searching for a solution, I ran across this set of patches:

  https://code.google.com/p/google-authenticator/issues/detail?id=192

I adapted it for security/pam_google_authenticator.  The patches are below

How-To-Repeat:
Attempt to use pam_google_authenticator with devices that use anything other than 30 second periods.

Fix:
Patch #1 for security/pam_google_authenticator/Makefile:

*** Makefile.orig       Fri Oct 31 20:35:18 2014
--- ./Makefile  Fri Oct 31 19:51:11 2014
***************
*** 3,8 ****
--- 3,9 ----
  
  PORTNAME=     pam_google_authenticator
  PORTVERSION=  20140826
+ PORTREVISION= 1
  CATEGORIES=   security
  MASTER_SITES= LOCAL/riggs/google-authenticator
  DISTNAME=     google-authenticator-${PORTVERSION}
***************
*** 12,21 ****
--- 13,31 ----
  
  LICENSE=      APACHE20
  
+ OPTIONS_DEFINE=       STEPSIZE
+ STEPSIZE_DESC=        Allow time steps other than the default of 30 seconds
+ 
  USES=         gmake
  
  PLIST_FILES=  bin/google-authenticator lib/pam_google_authenticator.so
  
+ .include <bsd.port.options.mk>
+ 
+ .if ${PORT_OPTIONS:MSTEPSIZE}
+ CFLAGS+=      -DSTEPSIZE
+ .endif
+ 
  do-install:
        ${INSTALL_PROGRAM} ${WRKSRC}/google-authenticator \
                ${STAGEDIR}${PREFIX}/bin/google-authenticator



I also created a patch for pam_google_authenticator.c based on the URL in this PR's description.  The following diff needs to be dropped into the port as security/pam_google_authenticator/files/patch-pam_google_authenticator.c.


------------------------------------CUT-HERE------------------------------------
*** pam_google_authenticator.c.orig     Thu Jan 30 15:17:38 2014
--- pam_google_authenticator.c  Fri Oct 31 19:42:43 2014
***************
*** 503,512 ****
  }
  #endif
  
- static int get_timestamp(void) {
-   return get_time()/30;
- }
- 
  static int comparator(const void *a, const void *b) {
    return *(unsigned int *)a - *(unsigned int *)b;
  }
--- 503,508 ----
***************
*** 538,543 ****
--- 534,574 ----
    return NULL;
  }
  
+ #if !defined(STEPSIZE)
+ static int get_timestamp(void) {
+   return get_time()/30;
+ }
+ #else
+ static int get_timestamp(pam_handle_t *pamh, const char *secret_filename,
+                        const char *buf) {
+   const char *value = get_cfg_value(pamh, "STEP_SIZE", buf);
+   if (!value) {
+     // Default step size is 30.
+     free((void *)value);
+     return 30;
+   } else if (value == &oom) {
+     // Out of memory. This is a fatal error.
+     return 0;
+   }
+ 
+   char *endptr;
+   errno = 0;
+   int step = (int)strtoul(value, &endptr, 10);
+   if (errno || !*value || value == endptr ||
+       (*endptr && *endptr != ' ' && *endptr != '\t' &&
+        *endptr != '\n' && *endptr != '\r') ||
+       step < 1 || step > 60) {
+     free((void *)value);
+     log_message(LOG_ERR, pamh, "Invalid STEP_SIZE option in \"%s\"",
+                 secret_filename);
+     return 0;
+   }
+   free((void *)value);
+ 
+   return get_time()/step;
+ }
+ #endif
+ 
  static int set_cfg_value(pam_handle_t *pamh, const char *key, const char *val,
                           char **buf) {
    size_t key_len = strlen(key);
***************
*** 1162,1168 ****
    }
  
    // Compute verification codes and compare them with user input
!   const int tm = get_timestamp();
    const char *skew_str = get_cfg_value(pamh, "TIME_SKEW", *buf);
    if (skew_str == &oom) {
      // Out of memory. This is a fatal error
--- 1193,1199 ----
    }
  
    // Compute verification codes and compare them with user input
!   const int tm = get_timestamp(pamh, secret_filename, *buf);
    const char *skew_str = get_cfg_value(pamh, "TIME_SKEW", *buf);
    if (skew_str == &oom) {
      // Out of memory. This is a fatal error
------------------------------------CUT-HERE------------------------------------


I tested these diffs with Feitian c200 TOTP tokens (http://www.ftsafe.com/product/otp/totp) and they work.
Comment 1 John Marino freebsd_committer 2014-10-31 21:01:22 UTC
please resubmit the patches as attachments and *PLEASE* provide patches with unified format, e.g. "diff -u", not this format which is both barely readable and bugzilla viewer can't handle it.
Comment 2 paul 2014-11-01 01:17:59 UTC
Created attachment 148843 [details]
Unified diff for security/pam_google_authenticator/Makefile
Comment 3 paul 2014-11-01 01:18:50 UTC
Created attachment 148844 [details]
Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c
Comment 4 paul 2014-11-01 01:19:57 UTC
Unified diffs are now attached.

Paul
Comment 5 John Marino freebsd_committer 2014-11-01 08:07:13 UTC
Many thanks!
I see as a bonus we convert an existing patch to unified format.

I'm tweaking the title and notifying the maintainer that he has two weeks to respond, otherwise it's automatically approved.
Comment 6 paul 2014-11-03 21:22:14 UTC
Created attachment 149004 [details]
(UPDATED) Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c

I found a problem with my original patch for pam_google_authenticator.c.  I forgot to wrap the call to get_timestamp() inside of #if defined(STEPSIZE).  This is necessary since one of the versions of get_timestamp() requires arguments and the other does not.

This is an updated version of the diffs for pam_google_authenticator.c
Comment 7 paul 2014-11-03 21:29:32 UTC
Created attachment 149005 [details]
(UPDATE, again) Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c

One more try to get the format of the diff correct.  Sorry for the multiple updates.
Comment 8 paul 2014-11-04 17:16:22 UTC
Created attachment 149035 [details]
Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c

One final change to fix a bug that I found.  When STEPSIZE is enabled, but the user is using a default, 30 second TOTP token, then authentication was using a static time instead of being based on the current time.
Comment 9 paul 2014-11-04 17:18:49 UTC
Created attachment 149036 [details]
Unified diff for security/pam_google_authenticator/files/patch-pam_google_authenticator.c

And in the proper format this time.
Comment 10 John Marino freebsd_committer 2014-11-14 12:59:37 UTC
I think you can replace

+.include <bsd.port.options.mk>
+
+.if ${PORT_OPTIONS:MSTEPSIZE}
+CFLAGS+=	-DSTEPSIZE
+.endif


with:

STEPSIZE_CFLAGS= -DSTEPSIZE

You might want to verify and resubmit.

FYI, this PR will receive a maintainer timeout very soon.
Comment 11 paul 2014-11-18 21:03:10 UTC
Created attachment 149563 [details]
Unified diff for security/pam_google_authenticator/Makefile

An updated version of the patch for security/pam_google_authenticator/Makefile that uses "STEPSIZE_CFLAGS" instead of ".if ${PORT_OPTIONS:MSTEPSIZE}"
Comment 12 John Marino freebsd_committer 2014-11-19 09:30:13 UTC
Thanks, I'll promote this -- due to feedback timeout, maintainer approval no longer required.

For the future, having a single diff that updates multiple files is more convenient for the submitter, thanks.
Comment 13 commit-hook freebsd_committer 2014-11-22 18:30:36 UTC
A commit references this bug:

Author: riggs
Date: Sat Nov 22 18:30:18 UTC 2014
New revision: 373085
URL: https://svnweb.freebsd.org/changeset/ports/373085

Log:
  Introduce non-default OPTION for variable time steps
  besides the 30 seconds default

  PR:		194723
  Submitted by:	paul@dokas.name
  Approved by:	maintainer timeout

Changes:
  head/security/pam_google_authenticator/Makefile
  head/security/pam_google_authenticator/files/patch-pam_google_authenticator.c
Comment 14 Thomas Zander freebsd_committer 2014-11-22 18:35:34 UTC
Committed, thanks!