Re: Commit crasher fix to libsoup?

[Nate Nielsen <nielsen-list memberwebs com>]

Dan Winship wrote:
> Nate Nielsen wrote:
>>> No... RFC 2617 says that any auth response containing a challenge must
>>> contain a realm token, so the server response here is invalid, and
>>> soup_auth_new_from_header_list() should be returning NULL rather than
>>> returning a SoupAuth with a NULL realm. 
>> RFC doesn't say that the app should crash. If a application using
>> libsoup segfaults on invalid input, then that would seem to me to be
>> security bug that needs to be fixed.
> 
> Right, as I said, I think the fix should be to return NULL from
> soup_auth_new_from_header_list() in this case. That would fix the crash,
> because then every SoupAuth would always have a realm, so the strcmp in
> soup-session would always be safe.

I should have read that more closely. Does the attached patch look good?

>>> What server is this that's
>>> sending that response back?
>> This is a proprietary app server. But that's not the point.
> 
> Well, it is, because if you wrote the server, then I can make soup
> ignore its malformed WWW-Authenticate response, and tell you to fix your
> server, and not feel guilty. Whereas if you don't control the server,
> then I either need to make libsoup cope with its bad response, or feel
> guilty about not doing it. :)

Well my client wrote it. It's been fixed. I just figured I'd fix libsoup
as well.

Cheers,
Nate

Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/libsoup/ChangeLog,v
retrieving revision 1.510
diff -u -3 -p -r1.510 ChangeLog
--- ChangeLog	10 Jul 2006 16:26:19 -0000	1.510
+++ ChangeLog	11 Jul 2006 22:04:01 -0000
@@ -1,3 +1,13 @@
+2006-07-10  Nate Nielsen  <nielsen memberwebs com>
+
+	* libsoup/soup-auth.c (soup_auth_get_realm): Ensure we always have realm.
+
+	* libsoup/soup-auth.c (soup_auth_new_from_header_list):
+	* libsoup/soup-auth.h:
+	* libsoup/soup-auth-basic.c (construct):
+	* libsoup/soup-auth-digest.c (construct): Ensure that SoupAuth object has 
+	valid fields set.
+
 2006-07-10  Dan Winship  <danw novell com>
 
 	* configure.in: 2.2.95.1, and bump SOUP_AGE/SOUP_CURRENT this
Index: libsoup/soup-auth-basic.c
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-auth-basic.c,v
retrieving revision 1.5
diff -u -3 -p -r1.5 soup-auth-basic.c
--- libsoup/soup-auth-basic.c	11 Apr 2005 20:42:06 -0000	1.5
+++ libsoup/soup-auth-basic.c	11 Jul 2006 22:04:03 -0000
@@ -17,7 +17,7 @@
 #include "soup-misc.h"
 #include "soup-uri.h"
 
-static void construct (SoupAuth *auth, const char *header);
+static gboolean construct (SoupAuth *auth, const char *header);
 static GSList *get_protection_space (SoupAuth *auth, const SoupUri *source_uri);
 static const char *get_realm (SoupAuth *auth);
 static void authenticate (SoupAuth *auth, const char *username, const char *password);
@@ -68,7 +68,7 @@ soup_auth_basic_class_init (SoupAuthBasi
 }
 
 
-static void
+static gboolean
 construct (SoupAuth *auth, const char *header)
 {
 	SoupAuthBasicPrivate *priv = SOUP_AUTH_BASIC_GET_PRIVATE (auth);
@@ -78,10 +78,13 @@ construct (SoupAuth *auth, const char *h
 
 	tokens = soup_header_param_parse_list (header);
 	if (!tokens)
-		return;
+		return FALSE;
 
 	priv->realm = soup_header_param_copy_token (tokens, "realm");
 	soup_header_param_destroy_hash (tokens);
+
+        /* Not valid unless a realm exists. RFC 2617 */
+        return (priv->realm != NULL);
 }
 
 static GSList *
Index: libsoup/soup-auth-digest.c
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-auth-digest.c,v
retrieving revision 1.12
diff -u -3 -p -r1.12 soup-auth-digest.c
--- libsoup/soup-auth-digest.c	10 Apr 2006 18:09:01 -0000	1.12
+++ libsoup/soup-auth-digest.c	11 Jul 2006 22:04:03 -0000
@@ -21,7 +21,7 @@
 #include "soup-misc.h"
 #include "soup-uri.h"
 
-static void construct (SoupAuth *auth, const char *header);
+static gboolean construct (SoupAuth *auth, const char *header);
 static GSList *get_protection_space (SoupAuth *auth, const SoupUri *source_uri);
 static const char *get_realm (SoupAuth *auth);
 static void authenticate (SoupAuth *auth, const char *username, const char *password);
@@ -148,7 +148,7 @@ decode_algorithm (const char *name)
 	return decode_data_type (algorithm_types, name);
 }
 
-static void
+static gboolean
 construct (SoupAuth *auth, const char *header)
 {
 	SoupAuthDigestPrivate *priv = SOUP_AUTH_DIGEST_GET_PRIVATE (auth);
@@ -159,7 +159,7 @@ construct (SoupAuth *auth, const char *h
 
 	tokens = soup_header_param_parse_list (header);
 	if (!tokens)
-		return;
+		return FALSE;
 
 	priv->nc = 1;
 	/* We're just going to do qop=auth for now */
@@ -190,6 +190,9 @@ construct (SoupAuth *auth, const char *h
 	g_free (tmp);
 
 	soup_header_param_destroy_hash (tokens);
+
+	/* Not valid unless a realm / nonce exists. RFC 2617 */
+	return (priv->realm != NULL && priv->nonce != NULL);
 }
 
 static GSList *
Index: libsoup/soup-auth.c
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-auth.c,v
retrieving revision 1.31
diff -u -3 -p -r1.31 soup-auth.c
--- libsoup/soup-auth.c	2 Apr 2006 21:26:27 -0000	1.31
+++ libsoup/soup-auth.c	11 Jul 2006 22:04:03 -0000
@@ -85,7 +85,11 @@ soup_auth_new_from_header_list (const GS
 	if (!auth)
 		return NULL;
 
-	SOUP_AUTH_GET_CLASS (auth)->construct (auth, header);
+	if (!SOUP_AUTH_GET_CLASS (auth)->construct (auth, header)) {
+		g_object_unref (auth);
+		auth = NULL;
+	}
+
 	return auth;
 }
 
@@ -136,9 +140,12 @@ soup_auth_get_scheme_name (SoupAuth *aut
 const char *
 soup_auth_get_realm (SoupAuth *auth)
 {
+	char *realm;
 	g_return_val_if_fail (SOUP_IS_AUTH (auth), NULL);
 
-	return SOUP_AUTH_GET_CLASS (auth)->get_realm (auth);
+	realm = SOUP_AUTH_GET_CLASS (auth)->get_realm (auth);
+	g_assert (realm);
+	return realm;
 }
 
 /**
Index: libsoup/soup-auth.h
===================================================================
RCS file: /cvs/gnome/libsoup/libsoup/soup-auth.h,v
retrieving revision 1.14
diff -u -3 -p -r1.14 soup-auth.h
--- libsoup/soup-auth.h	14 Jun 2005 15:34:21 -0000	1.14
+++ libsoup/soup-auth.h	11 Jul 2006 22:04:03 -0000
@@ -25,7 +25,7 @@ typedef struct {
 
 	const char *scheme_name;
 
-	void         (*construct)            (SoupAuth      *auth,
+	gboolean     (*construct)            (SoupAuth      *auth,
 					      const char    *header);
 
 	GSList *     (*get_protection_space) (SoupAuth      *auth,