Confirmed still an issue with 1.8.19b1

The following diff (pasted here inline at the expense of accurate tab vs. space) between .16 and .17 is the relevant diff.


--- sudo-1.8.16/plugins/sudoers/parse.c 2016-03-17 16:13:10.000000000 +0000
+++ sudo-1.8.17/plugins/sudoers/parse.c 2016-06-18 02:44:21.000000000 +0000
@@ -197,8 +197,8 @@
            SET(validated, VALIDATE_FAILURE);
        if (pwcheck == always && def_authenticate)
            SET(validated, FLAG_CHECK_USER);
-       else if (pwcheck == never || nopass == true)
-           def_authenticate = false;
+       else if (nopass == true)
+           SET(validated, FLAG_NOPASSWD);
        debug_return_int(validated);
     }

@@ -774,7 +774,7 @@
     if (match != NULL && !match->negated) {
        const int len = sudo_printf(SUDO_CONV_INFO_MSG, "%s%s%s\n",
            safe_cmnd, user_args ? " " : "", user_args ? user_args : "");
-       rval = len == -1 ? -1 : 0;
+       rval = len < 0 ? -1 : 0;
     }
 done:
     debug_return_int(rval);


If I revert that for 1.8.17 and rebuild/reinstall, 'sudo -v' for a user configured with NOPASSWD does not prompt for a password.

Only the first hunk in the prior diff is the relevant change.

This comes from changeset 51e2a5ecacc6 on 2016-04-19

https://www.sudo.ws/repos/sudo/rev/51e2a5ecacc6

In this case, I only have a sudoers file in place (no ldap configuration and sudo was not configured --with-ldap).  So this behavior contradicts the statements in that changeset commit message (at least as I am reading it).

The root issue I think is this bit in plugins/sudoers/sudoers.c in sudoers_policy_main():

            switch (pwcheck) {
            case all:
                if (!ISSET(validated, FLAG_NOPASSWD))
                    nopass = false;
                break;

In the 'all' case, nopass can never get set to true.  It is initialized at the top of the function to false.

Created attachment 486 [details]
[patch] fix verifypw=all and NOPASSWD

The attached patch alters the 'nopass' initialization to be true for the verifypw=all case.  Then (in the 'all' case) during the TAILQ_FOREACH loop if any nss doesn't have NOPASSWD, nopass will get set to false.  If they all have NOPASSWD, it will remain the initialized value in the 'all' case (true).

I'm unsure this is completely correct.  Can the the 'snl' list every be empty?  If so, that would leave 'nopass' set to true in the verifypw=all case.  And so, if the list can be empty, nopass should probably be initialized to false.

Also, this patch is against 1.8.17.

Mechanically, the patch applies to 1.8.19b1 as well.

And testing 1.8.19b1 + patch at run-time confirms it fixes this bug in the sudoers file-only case.  Untested in the sudoers file + ldap case (or other combinations).

Regression test results with 1.8.19b1 + patch:

% make check
for d in lib/util  plugins/group_file plugins/sudoers  plugins/system_group src include doc examples; do  (cd $d && exec make check) && continue;  exit $?;  done
parse_gids_test: 6 tests run, 0 errors, 100% success rate
strsplit_test: 29 tests run, 0 errors, 100% success rate
atofoo_test: FAIL: -2 != 4294967294
atofoo_test: FAIL: 4294967294 != 4294967294
atofoo_test: 23 tests run, 2 errors, 91% success rate
hltq_test: 19 tests run, 0 errors, 100% success rate
sudo_conf/test1: OK
sudo_conf/test2: OK
sudo_conf/test3: OK
sudo_conf/test4: OK
sudo_conf/test4 (stderr): OK
sudo_conf/test5: OK
sudo_conf/test5 (stderr): OK
sudo_conf/test6: OK
sudo_conf/test7: OK
sudo_conf: 9/9 tests passed; 0/9 tests failed
sudo_parseln/test1: OK
sudo_parseln/test2: OK
sudo_parseln/test3: OK
sudo_parseln/test4: OK
sudo_parseln/test5: OK
sudo_parseln/test6: OK
sudo_parseln: 6/6 tests passed; 0/6 tests failed
*** Error code 2

Thanks for the detailed debugging.  I think it is best to just initialize "nopass" to a non-boolean value and then only set it to true for pwcheck == all when NOPASSWD is present and nopass has not already been set.

Created attachment 487 [details]
Fix for "sudo -v" regression when verifypw=all and NOPASSWD is set

(In reply to Todd C Miller from comment #9)
> Created attachment 487 [details]
> Fix for "sudo -v" regression when verifypw=all and NOPASSWD is set

This patch looks like it has the same problem if snl is empty.  nopass will be left as -1 which is the same as true as far as C is concerned.

But maybe you're saying your patch is just simpler (and snl will never be empty).  If so, then it's fine with me.  Pulling the pwcheck assignment out of the foreach loop (as in my patch) is a minor efficiency gain, I think, since that should not change in the loop.

(In reply to John Hein from comment #10)
> (In reply to Todd C Miller from comment #9)
> > Created attachment 487 [details]
> > Fix for "sudo -v" regression when verifypw=all and NOPASSWD is set
> 
> This patch looks like it has the same problem if snl is empty. 
> nopass will be left as -1 which is the same as true as far as C is
> concerned.
> 
> But maybe you're saying your patch is just simpler (and snl will
> never be empty).  If so, then it's fine with me.  Pulling the
> pwcheck assignment out of the foreach loop (as in my patch) is a
> minor efficiency gain, I think, since that should not change in the
> loop.

Actually the two patches are a little different for the 'snl' empty case.  In your patch, it will be -1 for all verifypw settings - vs. the prior patch where it will only be true for verifypw=all.

But it's still the case that if snl is guaranteed to never be empty (or we don't care what nopass is if snl is empty), the misleading "true" setting for nopass doesn't matter.

The reason the pwcheck assignment is inside the loop is that it is theoretically possible for that value to change.  For instance, the LDAP backend can set arbitrary Defaults options on a per-entry basis.

The list of sudoers sources can never actually be empty since we error out early if that is the case.  However, in my diff, it is not a problem for nopass to be -1 since it is tested explicitly against true when setting def_authenticate.

Okay... I wasn't sure if a side effect of the lookup() could be to change the sudo_defs_table.

Yes, I saw the explicit test for nopass against true.  I was trying to say that I was concerned that being left as the initial -1 will effectively cause a de facto NOPASSWD if the snl list is empty.  But I wasn't sure that was possible (to be empty).  Where do you error out early if snl is empty?

I see the early exit now (in sudoers_policy_init) when "no valid sudoers sources found" triggers an early return which then presumably prevents sudoers_policy_main from being used (and thus preventing def_authenticate from being incorrectly set to false).

It wasn't obvious at first.  The large number of 'goto' in sudoers_policy_main make it a little hard to determine if this fairly complex function could tigger a side effect (such as setting a global like def_authenticate) before exiting early.  There are definitely some cases where global side effects will occur even if a goto triggers an early exit there.

Sorry about being paranoid - I don't know the intricacies of this code, so I wanted to make sure I didn't break something else with this change.

Fixed in sudo 1.8.19