Polkit-1 Shadow Patch

[David Zeuthen]

Hi,

On Fri, Mar 19, 2010 at 6:22 PM, Andrew Psaltis <ampsaltis at gmail.com> wrote:
> Hi again.
>
> After looking at the previous patch a little bit, it seems that there were
> some bugs in handling authorization errors, but did not have time to correct
> them until recently.  This (new) patch should correct them. The
> caveats/comments in the previous message still apply.

Sorry for not replying earlier but there are several problems here

 - you make the patch hard to review - real effort is needed to even take a
   quick glance at it. Use bugzilla (as the docs tell you to) - and never
   gzip patches.

 - patch is not in standard git patch format - which means real effort is
   needed to get it into the tree

 - there's white-space junk in the patch

 - the patch introduces compiler warnings.

As you can see, the first two complaints are kinda the reason that I'm
not responding until now - you just don't make it very easy to review.
And since I've been busy working on other stuff, things that are hard
to review goes in the end of the queue.

More detailed comments follows below. Note that most of these things
are minor and should be easy to fix - please follow up in bugzilla
with a patch that addresses these comments... and we'll take it from
there and get the patch in. Thanks!

>-	$(NULL)
>+	polkitagenthelperprivate.c polkitagenthelperprivate.h

You need to use trailing $(NULL) here.

> +polkit_agent_helper_1_SOURCES += $(NULL)

This isn't needed.

> +extern char *crypt ();

Really? You are throwing away type-safety here. Instead, do as
crypt(3) man page is suggesting (by defining _XOPEN_SOURCE).

Also, while compiling this code

  polkitagenthelper-shadow.c: In function ‘main’:
  polkitagenthelper-shadow.c:59: warning: implicit declaration of
function ‘clearenv’
  polkitagenthelper-shadow.c:59: warning: nested extern declaration of
‘clearenv’
  polkitagenthelper-shadow.c:63: warning: implicit declaration of
function ‘setenv’
  polkitagenthelper-shadow.c:63: warning: nested extern declaration of
‘setenv’
  polkitagenthelper-shadow.c: In function ‘shadow_authenticate’:
  polkitagenthelper-shadow.c:180: warning: implicit declaration of
function ‘usleep’
  polkitagenthelper-shadow.c:180: warning: nested extern declaration
of ‘usleep’

which is caused by missing includes. This probably also needs fixing
when using the pam backend.

> +  if(!shadow_authenticate (shadow))
> +static int
> +shadow_authenticate(struct spwd *shadow)

You need to use a gboolean here instead of abusing the ! operator and
the int type. The style is also wrong (need space between if and the
starting parenthesis).

> +  /* Speak PAM to the daemon, thanks to David Zeuthen for the idea. */

Technically you are just speaking to libpolkit-agent-1 (or whoever
launched the helper). No need for this comment.

+  /* Ask shadow about the user requesting authentication */
+  if ((shadow = getspnam (user_to_auth)) == NULL)

Split into separate lines.

+      fprintf(stderr, "polkit-agent-helper-1: could not get shadow
information for%.100s", user_to_auth);

Just use %s.

+  /* Check whether the user's password has expired */
+  time(&tm);

Lacks space between function name and starting parenthesis. Also tm is
a bad name, use 'now' instead as that is a lot more... logical. Also,
it's easier to read if you write it as

 now = time (NULL);

+  if( shadow->sp_max >= 0 && (shadow->sp_lstchg + shadow->sp_max) *
60 * 60 * 24 <= tm)

Lacks space between if and starting parenthesis.

> +  if( shadow->sp_inact >= 0 && (shadow->sp_lstchg + shadow->sp_max + shadow->sp_inact) * 60 * 60 * 24           <= tm)

Ditto. Seriously, what's with all the whitespace? It doesn't inspire a
lot of confidence to find random whitespace... and this is supposed to
be security-sensitive code.

>  if (strcmp (shadow->sp_pwdp, crypt (passwd, shadow->sp_pwdp)) != 0)
>    goto error;

This is really nasty.

 - crypt(3) may return NULL (e.g. ENOSYS, see the man page)
 - you make it look like crypt() has no side-effects while it
   actually changes passwd object

So, split this into multiple lines and check return value of crypt(3).
You also need to add a comment explaining why you use the salt that
you use.

> --- /dev/null
> +++ b/src/polkitagent/polkitagenthelperprivate.c

Lots of problems here. First of all, you need to include config.h
otherwise HAVE_CLEARENV won't work. Second of all, you need to include
stdlib.h for clearenv() in case HAVE_CLEARENV is set. Third, the
compiler warns about this file, if you had payed any attention to
warnings we wouldn't have these issues.

> +#ifdef POLKIT_AUTHW_PAM

Spelling error. Seriously, if you had tried building with pam (which
you need to) and looked at compiler warnings it would say something
about open_session() being defined but not used.

     David