http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #7 from dtucker at zip.com.au 2006-04-17 22:00 ------- (From update of attachment 807) This bit doesn't seem right:>+ /* Loop until the required authmethods are done */ >+ if (authenticated && options.required_auth1 != NULL) { >+ if (auth_remove_from_list(&options.required_auth1, >+ meth_name) != 1) >+ fatal("INTERNAL ERROR: authenticated method " >+ "\"%s\" not in required list \"%s\"", >+ meth_name, options.required_auth1); >+ debug2("do_authloop: required list now: %s", >+ options.required_auth1 == NULL ? >+ "DONE" : options.required_auth1); >+ authenticated = 0;Unless I'm misreading it, this can't complete. Once the last method in the list is successful, "authenticated" is set and options.required_auth1 is non-null. The final method is removed and "authenticated" set is zeroed so the loop goes around again... ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #8 from djm at mindrot.org 2006-04-17 22:12 ------- auth_remove_from_list() returns NULL when the list is empty, so the normal authentication path will continue. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #9 from dtucker at zip.com.au 2006-04-17 22:23 ------- (In reply to comment #8)> auth_remove_from_list() returns NULL when the list is empty, so the normal > authentication path will continue.No, auth_remove_from_list() sets options.required_auth1 to NULL, but by that time you're already inside the "if" block and authenticated is zeroed unconditionally. I have updated the patch to -current and this part doesn't seem to work, although auth1.c has changed a lot and it's quite possible I missed something merging the patch. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983
dtucker at zip.com.au changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #807 is|0 |1
obsolete| |
------- Comment #10 from dtucker at zip.com.au 2006-04-17 23:02 -------
Created an attachment (id=1121)
--> (http://bugzilla.mindrot.org/attachment.cgi?id=1121&action=view)
Update attachment 807 to -current.
Updated patch 807 to -current. No (deliberate :-) changes.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983
dtucker at zip.com.au changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #1121 is|0 |1
obsolete| |
------- Comment #11 from dtucker at zip.com.au 2006-04-17 23:05 -------
Created an attachment (id=1122)
--> (http://bugzilla.mindrot.org/attachment.cgi?id=1122&action=view)
Corrected patch 1121.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #12 from dtucker at zip.com.au 2006-04-17 23:09 ------- (From update of attachment 1122) I think this is what is needed:>+ /* Loop until the required authmethods are done */ >+ if (authenticated && options.required_auth1 != NULL) { >+ if (auth_remove_from_list(&options.required_auth1, >+ meth_name) != 1) >+ fatal("INTERNAL ERROR: authenticated method " >+ "\"%s\" not in required list \"%s\"", >+ meth_name, options.required_auth1); >+ debug2("do_authloop: required list now: %s", >+ options.required_auth1 == NULL ? >+ "DONE" : options.required_auth1);if (options.required_auth1 == NULL) return;>+ authenticated = 0;------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #13 from djm at mindrot.org 2006-04-17 23:21 ------- I don't follow: you only enter that block if authenticated is set and the list still has one or more entries. Once you are in the block, it is because you have hit a required method. When you exit the block, if you have completed all the required methods then the list is set to NULL and any subsequent authentication against an enabled method will succeed. Returning without clearing "authenticated" means that the last method in the list will succeed authentication, which is incorrect: these options are "required, but not sufficient" authentications to be completed first. ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983
dtucker at zip.com.au changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #1122 is|0 |1
obsolete| |
------- Comment #14 from dtucker at zip.com.au 2006-04-17 23:43 -------
Created an attachment (id=1123)
--> (http://bugzilla.mindrot.org/attachment.cgi?id=1123&action=view)
Fix a null pointer deref when given an invalid method name.
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #15 from dtucker at zip.com.au 2006-04-17 23:59 ------- (In reply to comment #13)> Returning without clearing "authenticated" means that the last method in the > list will succeed authentication, which is incorrect: these options are > "required, but not sufficient" authentications to be completed first.Ah, I think this is the difference. I thought that all listed methods needed to be completed, but this seems to mean that you need all of the listed methods plus one more? ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.
http://bugzilla.mindrot.org/show_bug.cgi?id=983 ------- Comment #16 from djm at mindrot.org 2006-04-18 14:40 ------- Yes, that is why I say they are "required, but not sufficient". Maybe this is too confusing... Perhaps we could do a syntax like: Authentications public-key,* hostbased,gssapi Where completion of an entire list completes authentication, and '*' (which may only appear at the end of a list) means "any method not yet completed". ------- You are receiving this mail because: ------- You are the assignee for the bug, or are watching the assignee.