[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d8261b5cd2709036ed0e50762db152fe@smtp.hushmail.com>
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.
