-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rewrite common.c: isdec*() #2951 #2979
Rewrite common.c: isdec*() #2951 #2979
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of formats fail with this (see bot checks)
Testing: BestCrypt [Jetico BestCrypt (.jbc) PKCS12 PBE (Whirlpool / SHA-1 to SHA-512) 32/64]... (2xOMP) FAILED (valid (before init))
Testing: 7z, 7-Zip (512K iterations) [SHA256 32/32 AES]... FAILED (valid ($7z$1$19$0$1122$8$732b59fd26896e410000000000000000$2955316379$192$183$7544a3a7ec3eb99a33d80e57907e28fb8d0e140ec8
They might actually need more than 9 digits, I haven't looked yet. If that's it, we need to either enhance your function or stop using it in those formats.
BTW here's how you can test your work:
./john -test=0
./john -test-full=0
They test stuff differently, both should be ran to completion.
We also have a "test suite": https://github.com/magnumripper/jtrTestSuite
src/common.c
Outdated
atoi64[ARCH_INDEX(*pos)] = pos - itoa64; | ||
memset(atoi64, 0x7F, sizeof(atoi64)); | ||
for (pos = itoa64; pos <= &itoa64[63]; pos++) | ||
atoi64[ARCH_INDEX(*pos)] = pos - itoa64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are introducing lots and lots of whitespace violations. Did you review your own commit at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magnumripper I will correct that. My IDEA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most importantly we indent with tabs, not spaces. It doesn't matter how many spaces you'd use, it's still not correct.
(magnum was quicker than me to make some of these points, but I'll post this already-typed comment anyway) 7z and bestcrypt formats failed testing at CircleCI - please look into this. Maybe they use these functions inappropriately? If so, this PR would probably need to fix that as well. Please do not make any whitespace changes with this PR. (It looks like there might be a tab width issue as well. I normally work on JtR source files assuming 8 char tabs.) There are too many commits here - I'd be silly to have those completely reworked commits even mentioned in our repository. magnum, what do you say? Although someone (Jim?) suggested documenting that these functions do not specifically test for valid inputs to |
That alone is no problem in a PR: We can squash them when merging - even from the web gui. So just keep adding commits to this one until all is fine. |
ok I will fix the tab and the comment. The commit overflows lol, I think will have to do a new fork then. I will look into the 7z and bestcrypt format. |
Oh ok @magnumripper |
The problem with bestcrypt is probably with its use of
I think the best fix is to drop those calls to |
In 7z, it's a 32-bit CRC, which its |
Dhiru added the We also need to add test vectors with "negative" CRC, so that if our related changes fail we'll learn right away rather than only when someone with one of those older scripts complains. |
src/common.c
Outdated
int x = atoi(q); | ||
sprintf(buf, "%d", x); | ||
return !strcmp(q,buf); | ||
if (q && *q == '-') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you add the q &&
here? If someone calls one of these functions with NULL
, we should crash, not hide the caller's error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@solardiz oh ok. I thought we should fail on NULL
. I will fix that.
src/common.c
Outdated
return !strcmp(q,buf); | ||
const char *p = q; | ||
do { | ||
if (!q || *p < '0' || *p > '9' || p - q >= 9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto re: !q ||
here.
If we really have code that must test data < MAX_INT (or MAX_UINT) and > 999999999, then we can simply have that code to it inline, and not use these functions. Checking a decimal CRC32 value would certainly be one instance where a full atou() number requirement is needed. |
See #2981 for this. |
After making the changes to the bestCrypt and 7z, i tested with
@magnumripper The testsuite you suggested is not working.
@jfoug do we have to implement a new function for checking a decimal CRC32 value? or there is something we can do to fix that. As solardiz said better to have one that work. |
This is why I was not 100% 'keen' on the length <= 9 'rule' for isdec()/isdecu() I was pretty sure there were instances wehre we needed to check up to val [0,0x80000000) and uval [0, 0xFFFFFFFF] Testing the range of [0,0x3B9AC9FF] (i.e. < 1 billion) works for 98% of the cases, but those other 2% can not be use that function. My idea from the start was to :
For the check for atoi length 10, it would be code like this
With code like that added (yes, ugly, BUT it works 100% of the time), we handle all valid 31 bit positive numbers. Then isdecu would check that any 10 digit character strings are <= 0xffffffff 4,294,967,295 The isdec_negok, is just a bit harder. You have to check that the number is <= 0x7fffffff if there was no negative sign, but <= 0x80000000 if there was a leading negative sign. But other than that, this is really the only change (yes, stupidly simplistic, and probably could be done in a better manner), BUT it would give us full range isdec[u]_negok checking, and would match what we were TRYING to do before, but depending upon UB behavior. |
Of course it does. Like the output suggests, you need to install some perl "libs". |
@jfoug Very good detailed idea. I will work on that. so for now I should revert all the changes solardiz suggested on the bestcrypt and 7z, try to implement this for a higher range. Thanks, Jim. will start working on it. @magnumripper I got it. You were right. |
@seekaddo the code you have is fine. It is just for the string where there exactly 10 digits. in that case, perform a check to make sure the number is < than a specific max. Once that is done, you have a full range isdec() function. NOTE, once done like this, you will not be able to use @seekaddo if there were changes made in 7z valid() (or other places) to work around the 999999999 limitation, then yes, all of those changes should be reverted, and then we simply fix isdec, et el to be compatible with what they were before (i.e. full 31 or 32 bit range). Yes, this does make assumptions that atoi handles 2's complement of at least 31 bit positive signed numbers. But that is a pretty safe assumption (IMHO) I think it would still be 100% valid if atoi handles 63 bit positive signed ints (say on a cray). It still should work just fine, since we are not making formats that are specific to 64 bit int validation rules. So all validation using these is already assuming 31/32 bits, so using those constants should put us right back in the exact logic replacement category for these functions. |
We've just found some real issues in those two formats due to this attempt. Are we going to merely relax the functions that enabled this?! I think the range between 9 9's and 2 GiB or 4 GiB is rather narrow to be worth enabling it everywhere, with the integer overflow risks where the value returned by But I don't feel about this strongly. So I'm fine with JimF's suggestion winning; I'll just stay out of this discussion from this point on. That's actually good news to me - I got other uses for my time, and it looks like @seekaddo might be willing to contribute to our project in other ways going forward as well, so the primary mission of gaining a contributor might be accomplished. :-) |
BTW, Jim is the person who wrote the |
IOW, for this PR to be consistent, if we make |
@solardiz is that for latest 7z format being able to read non-latest 7z2john hashes? I'm not sure I disagree yet, just asking. |
Yes, the needed 7z fix is "for latest 7z format being able to read non-latest 7z2john hashes". This may be a rare occasion, except that now we also have a test vector for it, so if |
From reference:
So what we have within our isdec, is trying to determine if the string represents an int value, from 0 to MAX_INT of from 0 to 0x7FFFFFFF if MAX_INT is > 0x7FFFFFFF so I see nothing undefined here. If we are building john for a system where MAX_INT is < 0x7FFFFFFF (such as 0x7FFF), then the problems caused by our assumption will be meaningless, compared to the 100's to 1000's of other failure points in the source. making isdec return 1 for p[0 .. 0x80000000) causes nothing to be undefined if sizeof(int) is 8 It simply means that only smaller data (but same size as used everywhere) will ever be tested against atoi |
I certainly agree here, that using atou(-500) is very bad certainly undefined. If that is how we are using it, then we should change that. that is 'works' on some systems is meaningless. it returns unsigned, and -500 is not in the defined range. |
Btw, atou is 100% owned code inside of jumbo, it is this (in misc.c) unsigned atou(const char *src) {
unsigned val;
sscanf(src, "%u", &val);
return val;
} Now the undefined part is sscanf("-500", "%u", &val); I am pretty sure that using %u with a negative string is fully undefined. I could be wrong on that, but i would REALLY be surprised. |
Jim, we're in agreement on this - |
LOL, this is certainly not a beginner's issue. We should have realised there'd be dragons inside. |
That does not fly at all. It would return false for 1200000000 already. And even 1008000000. |
What do you mean? I will try this out |
I mean once the "Files changed" tab that I mentioned show no irrelevant changes nor any whitespace violations, I can squash & merge and it will look like a single commit. But as long as you keep all the bad and unrelated stuff in there, I won't merge it. |
@magnumripper ok, I got you. |
e14d96b
to
c4dc856
Compare
@magnumripper Is everything good now. |
No, did you think so? Try looking at https://github.com/magnumripper/JohnTheRipper/pull/2979/files for example. You are supposed to change only the functions we're talking about here, nothing else in these files. Also, you still indent your stuff with spaces as opposed to tabs. You seem to lack some basic knowledge about diffs and/or git and I'm not sure just what I need to explain to you. |
fec5345
to
d052831
Compare
@magnumripper done 😊😀😊 |
@magnumripper I hope everything is ok now. I have fixed the tabs problem also. If anything is not in order please let me know. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very close to being ready. There are just some coding style changes left to be made.
src/common.c
Outdated
sprintf(buf, "%d", x); | ||
return !strcmp(q,buf) && *q != '-'; | ||
const char *imax = "2147483647"; | ||
return isdec_len(q, imax); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to define imax
, you can use simply:
return isdec_len(q, "2147483647");
src/common.c
Outdated
int x = atoi(q); | ||
sprintf(buf, "%d", x); | ||
return !strcmp(q,buf); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded empty line.
src/common.c
Outdated
if (*q != '-') | ||
return isdec(q); | ||
|
||
return isdec_len(q + 1, imx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also no need to define imx
, and you can optionally simplify the whole function body to:
return *q == '-' ? isdec_len(q + 1, "2147483648") : isdec(q);
src/common.c
Outdated
{ | ||
const char *p = q; | ||
do | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't normally put braces on their own lines, except for braces around function bodies. So this would be:
do {
src/common.c
Outdated
} while (*++p); | ||
|
||
return p - q < 10 || strcmp(q, mxv) <= 0; | ||
} | ||
int isdec(const char *q) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need empty line before new function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/common.c
Outdated
sprintf(buf, "%d", x); | ||
return !strcmp(q,buf) && *q != '-'; | ||
const char *imax = "2147483647"; | ||
return isdec_len(q, imax); | ||
} | ||
int isdec_negok(const char *q) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need empty line before new function.
src/common.c
Outdated
if (*q != '-') | ||
return isdec(q); | ||
|
||
return isdec_len(q + 1, imx); | ||
} | ||
int isdecu(const char *q) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need empty line before new function.
src/common.c
Outdated
return !strcmp(q,buf); | ||
const char *umax = "4294967295"; | ||
|
||
return isdec_len(q, umax); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to define umax
, you can use simply:
return isdec_len(q, "4294967295");
src/common.h
Outdated
@@ -129,11 +129,12 @@ int ishexlcn(const char *q, int n); | |||
size_t hexlen(const char *q, int *extra_chars); | |||
size_t hexlenl(const char *q, int *extra_chars); /* lower cased only */ | |||
size_t hexlenu(const char *q, int *extra_chars); /* upper cased only */ | |||
/* is this a valid string for atoi() ONLY positive numbers are valid */ | |||
/*Is this a valid number. | |||
*ONLY positive numbers are valid. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a single white space after the asterisks at the start of comment lines, so:
/* Is this a valid number.
* ONLY positive numbers are valid. */
src/common.h
Outdated
int isdec_negok(const char *q); | ||
/* is this a valid string for atou()? atou() func == sprintf("%x",&val) */ | ||
/* Is this a valid number*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are non-descriptive (same comment for both functions, then what's the difference between them?), and besides the (lack of) whitespace before/after asterisks is sloppy.
Getting very close now! Fix everything Solar said, and then we may actually be able to merge this 😄 |
@solardiz @magnumripper Working on it. |
127b570
to
a23f536
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really close now ;-)
src/common.c
Outdated
return !strcmp(q,buf); | ||
} | ||
return isdec_len(q, "4294967295"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing last linefeed
a23f536
to
fd5f555
Compare
@magnumripper hahaha done. |
Aaaand... merged! Thanks. |
@magnumripper aaaaah "naaa endlich" -> Finally. Thank you guys for the patience, and all the support. You are golden. @solardiz @jfoug |
Thank you for staying with us through all of this, @seekaddo! The function descriptions in the .h file still look sloppy to me, but no more so than much of the rest of jumbo, and we got to stop somewhere. Now we should probably open a new issue for 7z format's misuse of "%u" on a negative number. I expect it will be causing self-test failures on some platforms, so the current state is not really suitable for release. @seekaddo please feel free to open that issue and optionally work on it once open, or you may look into other issues we have. Thanks again. |
@seekaddo Good job, and great that you stayed this issue to the end. Keep this memory about of doing things the right way, vs hacking something out that works, but can not be shown to work in all places, which is the situation you started from! :) If you need help in other code, just ask. I purposely tried to point you in the right direction on this bug, without spoon feeding you the final answer. But that will not always be the case. If you get stuck, I can easily help get you over a hump, and let you continue. Especially large humps are just figuring out how the core code in john loads and uses the formats themselves. Starting with a fix to a formats valid() method (like Solar listed for the undefined usage of %u for a possible signed item), is a good way to get your feet wet and see how a format works, and to fix something that really is very wrong. |
@solardiz Thanks for the support. I have really learnt a lot more than in a Semester. I love it. I have also been going through the whole |
@solardiz ok I will do that. |
I have tested the three functions and everything is working well.
Thanks