From cf377b1b6d6a0e9d5b622d48b0617a9416ea574e Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 24 Feb 2012 14:50:52 -0500 Subject: [PATCH] SoupAuthManagerNTLM: fix don't-fallback-to-Basic code Sessions using NTLM should never fall back to using Basic auth to access a resource which advertises NTLM auth. But ntlm-test wasn't actually testing this, and it broke at some point. Fix that, and add tests to ntlm-test. --- libsoup/soup-auth-manager-ntlm.c | 12 ++-- libsoup/soup-auth-manager.c | 94 ++++++++++++++++++++-------- libsoup/soup-auth-manager.h | 11 ++-- tests/ntlm-test.c | 132 ++++++++++++++++++++++++++------------- 4 files changed, 172 insertions(+), 77 deletions(-) diff --git a/libsoup/soup-auth-manager-ntlm.c b/libsoup/soup-auth-manager-ntlm.c index 33043e7..cf5218b 100644 --- a/libsoup/soup-auth-manager-ntlm.c +++ b/libsoup/soup-auth-manager-ntlm.c @@ -388,6 +388,7 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm) SOUP_AUTH_MANAGER_NTLM_GET_PRIVATE (ntlm); SoupNTLMConnection *conn; const char *val; + char *challenge = NULL; SoupURI *uri; conn = get_connection_for_msg (priv, msg); @@ -396,10 +397,11 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm) val = soup_message_headers_get_list (msg->response_headers, "WWW-Authenticate"); - if (val) - val = strstr (val, "NTLM "); if (!val) return; + challenge = soup_auth_manager_extract_challenge (val, "NTLM"); + if (!challenge) + return; if (conn->state > SOUP_NTLM_SENT_REQUEST) { /* We already authenticated, but then got another 401. @@ -410,7 +412,7 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm) goto done; } - if (!soup_ntlm_parse_challenge (val, &conn->nonce, &conn->domain)) { + if (!soup_ntlm_parse_challenge (challenge, &conn->nonce, &conn->domain)) { conn->state = SOUP_NTLM_FAILED; goto done; } @@ -418,7 +420,7 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm) conn->auth = soup_auth_ntlm_new (conn->domain, soup_message_get_uri (msg)->host); #ifdef USE_NTLM_AUTH - conn->challenge_header = g_strdup (val + 5); + conn->challenge_header = g_strdup (challenge + 5); if (conn->state == SOUP_NTLM_SENT_SSO_REQUEST) { conn->state = SOUP_NTLM_RECEIVED_SSO_CHALLENGE; goto done; @@ -435,6 +437,8 @@ ntlm_authorize_pre (SoupMessage *msg, gpointer ntlm) } done: + g_free (challenge); + /* Remove the WWW-Authenticate headers so the session won't try * to do Basic auth too. */ diff --git a/libsoup/soup-auth-manager.c b/libsoup/soup-auth-manager.c index 12e3f4e..1aacf48 100644 --- a/libsoup/soup-auth-manager.c +++ b/libsoup/soup-auth-manager.c @@ -241,37 +241,84 @@ auth_header_for_message (SoupMessage *msg) } } -static char * -extract_challenge (const char *challenges, const char *scheme) +static GSList * +next_challenge_start (GSList *items) { - GSList *items, *i; - int schemelen = strlen (scheme); - char *item, *space, *equals; - GString *challenge; - - /* The relevant grammar: + /* The relevant grammar (from httpbis): * * WWW-Authenticate = 1#challenge * Proxy-Authenticate = 1#challenge - * challenge = auth-scheme 1#auth-param + * challenge = auth-scheme [ 1*SP ( b64token / #auth-param ) ] * auth-scheme = token - * auth-param = token "=" ( token | quoted-string ) + * auth-param = token BWS "=" BWS ( token / quoted-string ) + * b64token = 1*( ALPHA / DIGIT / + * "-" / "." / "_" / "~" / "+" / "/" ) *"=" * * The fact that quoted-strings can contain commas, equals * signs, and auth scheme names makes it tricky to "cheat" on - * the parsing. We just use soup_header_parse_list(), and then - * reassemble the pieces after we find the one we want. + * the parsing. So soup_auth_manager_extract_challenge() will + * have used soup_header_parse_list() to split the header into + * items. Given the grammar above, the possible items are: + * + * auth-scheme + * auth-scheme 1*SP b64token + * auth-scheme 1*SP auth-param + * auth-param + * + * where the first three represent the start of a new challenge and + * the last one does not. */ + for (; items; items = items->next) { + const char *item = items->data; + const char *sp = strpbrk (item, "\t\r\n "); + const char *eq = strchr (item, '='); + + if (!eq) { + /* No "=", so it can't be an auth-param */ + return items; + } + if (!sp || sp > eq) { + /* No space, or first space appears after the "=", + * so it must be an auth-param. + */ + continue; + } + while (g_ascii_isspace (*++sp)) + ; + if (sp == eq) { + /* First "=" appears immediately after the first + * space, so this must be an auth-param with + * space around the "=". + */ + continue; + } + + /* "auth-scheme auth-param" or "auth-scheme b64token" */ + return items; + } + + return NULL; +} + +char * +soup_auth_manager_extract_challenge (const char *challenges, const char *scheme) +{ + GSList *items, *i, *next; + int schemelen = strlen (scheme); + char *item; + GString *challenge; + items = soup_header_parse_list (challenges); - /* First item will start with the scheme name, followed by a - * space and then the first auth-param. + /* First item will start with the scheme name, followed by + * either nothing, or else a space and then the first + * auth-param. */ - for (i = items; i; i = i->next) { + for (i = items; i; i = next_challenge_start (i->next)) { item = i->data; if (!g_ascii_strncasecmp (item, scheme, schemelen) && - g_ascii_isspace (item[schemelen])) + (!item[schemelen] || g_ascii_isspace (item[schemelen]))) break; } if (!i) { @@ -279,17 +326,10 @@ extract_challenge (const char *challenges, const char *scheme) return NULL; } - /* The challenge extends from this item until the end, or until - * the next item that has a space before an equals sign. - */ + next = next_challenge_start (i->next); challenge = g_string_new (item); - for (i = i->next; i; i = i->next) { + for (i = i->next; i != next; i = i->next) { item = i->data; - space = strpbrk (item, " \t"); - equals = strchr (item, '='); - if (!equals || (space && equals > space)) - break; - g_string_append (challenge, ", "); g_string_append (challenge, item); } @@ -313,7 +353,7 @@ create_auth (SoupAuthManagerPrivate *priv, SoupMessage *msg) for (i = priv->auth_types->len - 1; i >= 0; i--) { auth_class = priv->auth_types->pdata[i]; - challenge = extract_challenge (header, auth_class->scheme_name); + challenge = soup_auth_manager_extract_challenge (header, auth_class->scheme_name); if (challenge) break; } @@ -336,7 +376,7 @@ check_auth (SoupMessage *msg, SoupAuth *auth) if (!header) return FALSE; - challenge = extract_challenge (header, soup_auth_get_scheme_name (auth)); + challenge = soup_auth_manager_extract_challenge (header, soup_auth_get_scheme_name (auth)); if (!challenge) return FALSE; diff --git a/libsoup/soup-auth-manager.h b/libsoup/soup-auth-manager.h index 493960a..d82fbb1 100644 --- a/libsoup/soup-auth-manager.h +++ b/libsoup/soup-auth-manager.h @@ -32,10 +32,13 @@ typedef struct { GType soup_auth_manager_get_type (void); -void soup_auth_manager_emit_authenticate (SoupAuthManager *manager, - SoupMessage *msg, - SoupAuth *auth, - gboolean retrying); +void soup_auth_manager_emit_authenticate (SoupAuthManager *manager, + SoupMessage *msg, + SoupAuth *auth, + gboolean retrying); + +char *soup_auth_manager_extract_challenge (const char *challenges, + const char *scheme); G_END_DECLS diff --git a/tests/ntlm-test.c b/tests/ntlm-test.c index 46ac46e..f5462c6 100644 --- a/tests/ntlm-test.c +++ b/tests/ntlm-test.c @@ -58,29 +58,25 @@ server_callback (SoupServer *server, SoupMessage *msg, SoupSocket *socket; const char *auth; NTLMServerState state, required_user = 0; - gboolean auth_required = FALSE, not_found = FALSE; - gboolean basic_allowed = FALSE, ntlm_allowed = FALSE; + gboolean auth_required, not_found = FALSE; + gboolean basic_allowed = TRUE, ntlm_allowed = TRUE; if (msg->method != SOUP_METHOD_GET) { soup_message_set_status (msg, SOUP_STATUS_NOT_IMPLEMENTED); return; } - if (!strncmp (path, "/alice", 6)) { - auth_required = TRUE; - ntlm_allowed = TRUE; + if (!strncmp (path, "/alice", 6)) required_user = NTLM_AUTHENTICATED_ALICE; - } else if (!strncmp (path, "/bob", 4)) { - auth_required = TRUE; - ntlm_allowed = TRUE; + else if (!strncmp (path, "/bob", 4)) required_user = NTLM_AUTHENTICATED_BOB; - } else if (!strncmp (path, "/either", 7)) { - auth_required = TRUE; - ntlm_allowed = basic_allowed = TRUE; - } else if (!strncmp (path, "/basic", 6)) { - auth_required = TRUE; - basic_allowed = TRUE; - } + else if (!strncmp (path, "/either", 7)) + ; + else if (!strncmp (path, "/basic", 6)) + ntlm_allowed = FALSE; + else if (!strncmp (path, "/noauth", 7)) + basic_allowed = ntlm_allowed = FALSE; + auth_required = ntlm_allowed || basic_allowed; if (strstr (path, "/404")) not_found = TRUE; @@ -95,7 +91,9 @@ server_callback (SoupServer *server, SoupMessage *msg, if (!strncmp (auth + 5, NTLM_REQUEST_START, strlen (NTLM_REQUEST_START))) { state = NTLM_RECEIVED_REQUEST; - /* If they start, they must finish */ + /* If they start, they must finish, even if + * it was unnecessary. + */ auth_required = ntlm_allowed = TRUE; basic_allowed = FALSE; } else if (state == NTLM_SENT_CHALLENGE && @@ -104,12 +102,15 @@ server_callback (SoupServer *server, SoupMessage *msg, state = NTLM_RESPONSE_USER (auth + 5); } else state = NTLM_UNAUTHENTICATED; - } else if (!strncmp (auth, "Basic ", 6) && basic_allowed) { + } else if (basic_allowed && !strncmp (auth, "Basic ", 6)) { gsize len; char *decoded = (char *)g_base64_decode (auth + 6, &len); - if (!strncmp (decoded, "alice:password", len) || - !strncmp (decoded, "bob:password", len)) + if (!strncmp (decoded, "alice:password", len) && + required_user != NTLM_AUTHENTICATED_BOB) + auth_required = FALSE; + else if (!strncmp (decoded, "bob:password", len) && + required_user != NTLM_AUTHENTICATED_ALICE) auth_required = FALSE; g_free (decoded); } @@ -122,13 +123,13 @@ server_callback (SoupServer *server, SoupMessage *msg, if (auth_required) { soup_message_set_status (msg, SOUP_STATUS_UNAUTHORIZED); - if (basic_allowed) { + if (basic_allowed && state != NTLM_RECEIVED_REQUEST) { soup_message_headers_append (msg->response_headers, "WWW-Authenticate", "Basic realm=\"ntlm-test\""); } - if (state == NTLM_RECEIVED_REQUEST) { + if (ntlm_allowed && state == NTLM_RECEIVED_REQUEST) { soup_message_headers_append (msg->response_headers, "WWW-Authenticate", "NTLM " NTLM_CHALLENGE); @@ -158,7 +159,8 @@ static void authenticate (SoupSession *session, SoupMessage *msg, SoupAuth *auth, gboolean retrying, gpointer user) { - soup_auth_authenticate (auth, user, "password"); + if (!retrying) + soup_auth_authenticate (auth, user, "password"); } typedef struct { @@ -180,11 +182,9 @@ prompt_check (SoupMessage *msg, gpointer user_data) "WWW-Authenticate"); if (header && strstr (header, "Basic ")) state->got_basic_prompt = TRUE; - if (!state->sent_ntlm_request) { - if (header && strstr (header, "NTLM") && - !strstr (header, NTLM_CHALLENGE)) - state->got_ntlm_prompt = TRUE; - } + if (header && strstr (header, "NTLM") && + !strstr (header, NTLM_CHALLENGE)) + state->got_ntlm_prompt = TRUE; } static void @@ -333,10 +333,11 @@ static void do_ntlm_round (SoupURI *base_uri, gboolean use_ntlm, const char *user) { SoupSession *session; - gboolean alice = use_ntlm && !strcmp (user, "alice"); - gboolean bob = use_ntlm && !strcmp (user, "bob"); - - g_return_if_fail (use_ntlm || !alice); + gboolean alice = !g_strcmp0 (user, "alice"); + gboolean bob = !g_strcmp0 (user, "bob"); + gboolean alice_via_ntlm = use_ntlm && alice; + gboolean bob_via_ntlm = use_ntlm && bob; + gboolean alice_via_basic = !use_ntlm && alice; session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL); if (use_ntlm) @@ -347,40 +348,87 @@ do_ntlm_round (SoupURI *base_uri, gboolean use_ntlm, const char *user) G_CALLBACK (authenticate), (char *)user); } + /* 1. Server doesn't request auth, so both get_ntlm_prompt and + * get_basic_prompt are both FALSE, and likewise do_basic. But + * if we're using NTLM we'll try that even without the server + * asking. + */ do_message (session, base_uri, "/noauth", FALSE, use_ntlm, FALSE, FALSE, SOUP_STATUS_OK); + + /* 2. Server requires auth as Alice, so it will request that + * if we didn't already authenticate the connection to her in + * the previous step. If we authenticated as Bob in the + * previous step, then we'll just immediately get a 401 here. + * So in no case will we see the client try to do_ntlm. + */ do_message (session, base_uri, "/alice", - !use_ntlm || bob, FALSE, - FALSE, FALSE, + !alice_via_ntlm, FALSE, + !alice_via_ntlm, alice_via_basic, alice ? SOUP_STATUS_OK : SOUP_STATUS_UNAUTHORIZED); + + /* 3. Server still requires auth as Alice, but this URI + * doesn't exist, so Alice should get a 404, but others still + * get 401. Alice-via-NTLM is still authenticated, and so + * won't get prompts, and Alice-via-Basic knows at this point + * to send auth without it being requested, so also won't get + * prompts. But Bob/nobody will. + */ do_message (session, base_uri, "/alice/404", - !use_ntlm, bob, - FALSE, FALSE, + !alice, bob_via_ntlm, + !alice, alice_via_basic, alice ? SOUP_STATUS_NOT_FOUND : SOUP_STATUS_UNAUTHORIZED); + + /* 4. Should be exactly the same as #3, except the status code */ do_message (session, base_uri, "/alice", - !use_ntlm, bob, - FALSE, FALSE, + !alice, bob_via_ntlm, + !alice, alice_via_basic, alice ? SOUP_STATUS_OK : SOUP_STATUS_UNAUTHORIZED); + + /* 5. This path requires auth as Bob; Alice-via-NTLM will get + * an immediate 401 and not try to reauthenticate. + * Alice-via-Basic will get a 401 and then try to do Basic + * (and fail). Bob-via-NTLM will try to do NTLM right away and + * succeed. + */ do_message (session, base_uri, "/bob", - !use_ntlm || alice, bob, - FALSE, FALSE, + !bob_via_ntlm, bob_via_ntlm, + !bob_via_ntlm, alice_via_basic, bob ? SOUP_STATUS_OK : SOUP_STATUS_UNAUTHORIZED); + + /* 6. Back to /alice. Somewhat the inverse of #5; Bob-via-NTLM + * will get an immediate 401 and not try again, Alice-via-NTLM + * will try to do NTLM right away and succeed. Alice-via-Basic + * still knows about this path, so will try Basic right away + * and succeed. + */ do_message (session, base_uri, "/alice", - !use_ntlm || bob, alice, - FALSE, FALSE, + !alice_via_ntlm, alice_via_ntlm, + !alice_via_ntlm, alice_via_basic, alice ? SOUP_STATUS_OK : SOUP_STATUS_UNAUTHORIZED); + + /* 7. Server accepts Basic auth from either user, but not NTLM. + * Since Bob-via-NTLM is unauthenticated at this point, he'll try + * NTLM before realizing that the server doesn't support it. + */ do_message (session, base_uri, "/basic", - FALSE, bob, + FALSE, bob_via_ntlm, TRUE, user != NULL, user != NULL ? SOUP_STATUS_OK : SOUP_STATUS_UNAUTHORIZED); + + /* 8. Server accepts Basic or NTLM from either user. + * Alice-via-NTLM is still authenticated at this point from #6, + * and Bob-via-NTLM is authenticated from #7, so neither + * of them will do anything. + */ do_message (session, base_uri, "/either", !use_ntlm, FALSE, !use_ntlm, !use_ntlm && user != NULL, -- 2.7.4