Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 10 Dec 2012 02:37:41 +0100
From: magnum <john.magnum@...hmail.com>
To: john-dev@...ts.openwall.com
Subject: Re: fixing the valid() methods

On 10 Dec, 2012, at 1:27 , Solar Designer <solar@...nwall.com> wrote:
> (...)and Dhiru is now working on fixing them, usually "by copying get_salt
> logic to valid" (in his words).  Does this mean code duplication?  If
> so, that's not great.  Also, were not some of the crashes in get_salt()
> itself?  If so, more robust code needs to be written for valid(), not
> the overly trusting code copied from get_salt().

I was going to raise a warning flag too. Here's an example:

diff --git a/src/ssh_ng_fmt_plug.c b/src/ssh_ng_fmt_plug.c
index b4c4f16..83ae58a 100644
--- a/src/ssh_ng_fmt_plug.c
+++ b/src/ssh_ng_fmt_plug.c
@@ -81,7 +81,34 @@ static void init(struct fmt_main *self)
 
 static int valid(char *ciphertext, struct fmt_main *self)
 {
-       return !strncmp(ciphertext, "$sshng$", 7);
+       char *ctcopy = strdup(ciphertext);
+       char *keeptr = ctcopy;
+       char *p;
+       int ctlen;
+       if (strncmp(ciphertext, "$sshng$", 7) != 0)
+               goto err;
+       ctcopy += 7;
+       p = strtok(ctcopy, "$"); /* cipher */
+       if ((p = strtok(NULL, "$")) == NULL)    /* salt len */
+               goto err;
+       if(atoi(p) > 16)
+               goto err;
+       if ((p = strtok(NULL, "$")) == NULL)    /* salt */
+               goto err;
+       if ((p = strtok(NULL, "$")) == NULL)    /* ciphertext length */
+               goto err;
+       ctlen = atoi(p);
+       if ((p = strtok(NULL, "$")) == NULL)    /* ciphertext */
+               goto err;
+       if(strlen(p) != ctlen * 2)
+               goto err;
+
+       MEM_FREE(keeptr);
+       return 1;
+
+err:
+       MEM_FREE(keeptr);
+       return 0;
 }

1. It's definitely suboptimal to do a strdup() before even checking the format tag. Is it not true that when loading eg. KoreLogic's millions-of-hashes files (and not specifying --format) each and every input line will be sent to every valid() of all the 200+ formats? I think that is what happens. Have this in mind when implementing valid()...

2. I dislike strdup/free if you can avoid allocating at all... but I'm not sure it's that much a problem provided you fix #1. I tend to use strchr/strrchr on the original string instead.

magnum

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.