-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
maint(gpg2john.c): add missing prototypes #5603
maint(gpg2john.c): add missing prototypes #5603
Conversation
bots are happy! |
That's great! I'll have to let @solardiz approve it though, because I really don't understand any of it 😆 |
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.
I made review comments about implementation detail of your approach, but I also question the whole approach.
It'd be simpler to cast the two different-prototype function addresses to (funcptr)
in the initializer and then cast them to the right function pointer type in the two calls. A drawback is we wouldn't let the compiler check that we use the right type for those functions there - we'd take responsibility that we do.
src/gpg2john.c
Outdated
@@ -1295,7 +1295,7 @@ TAG[] = { | |||
#define TAG_NUM (sizeof(TAG) * sizeof(string)) | |||
|
|||
private void | |||
(*tag_func[])() = { | |||
(*tag_func[])(int) = { |
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.
Could use the funcptr
type here.
src/gpg2john.c
Outdated
@@ -1629,7 +1697,7 @@ parse_packet(char *hash) | |||
if (tag < TAG_NUM && tag_func[tag] != NULL) { | |||
if (gpg_dbg) | |||
fprintf(stderr, "Packet type %d, len %d at offset %d (Processing) (pkt-type %s) (Partial %s)\n", tag, len, offset, pkt_type(tag), partial?"yes":"no"); | |||
(*tag_func[tag])(len, 1, partial, hash); // first packet (possibly only one if partial is false). | |||
(*tag_func1[tag])(len, 1, partial, hash); // first packet (possibly only one if partial is false). |
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're now calling through a tag_func1
element, but the NULL check above remained of tag_func
- that's probably wrong now.
src/gpg2john.c
Outdated
@@ -1651,7 +1719,7 @@ parse_packet(char *hash) | |||
if (tag < TAG_NUM && tag_func[tag] != NULL) { | |||
if (gpg_dbg) | |||
fprintf(stderr, "Packet type %d, len %d at offset %d (Processing) (pkt-type %s) (Partial %s)\n", tag, len, offset, pkt_type(tag), partial?"yes":"no"); | |||
(*tag_func[tag])(len, 0, partial, hash); // subsquent packets. | |||
(*tag_func1[tag])(len, 0, partial, hash); // subsquent packets. |
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.
5e8bffe
to
0251a0f
Compare
|
It would be nice IMHO if you reference #5268 if you force-push again. Better traceability when trying to understand historical changes two years from now. |
4289de9
to
0b01ba0
Compare
No. Let's rephrase it: it calls some functions that expect only one parameter, but it passes 4 parameters. |
Testing now (from john-samples): Committed code: john-the-ripper.gpg2john GPG/test.utf8.asc > 1.hash
john-the-ripper.gpg2john GPG/gpg-70_flavors/*.sec >> 1.hash
cat 1.hash | LANG=C sort > old.hash PR: ../run/gpg2john GPG/test.utf8.asc > 2.hash
../run/gpg2john GPG/gpg-70_flavors/*.sec >> 2.hash
cat 2.hash | LANG=C sort > pr.hash diff old.hash pr.hash |
Both versions are producing the same result (see the tests above). |
0b01ba0
to
6614733
Compare
if (tag_func[tag] != NULL) | ||
(*tag_func[tag])(len); | ||
else | ||
(*tag_func4[tag])(len, 1, partial, hash); // first packet (possibly only one if partial is false). |
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.
None of the john-samples
examples are calling this else part. Anyway, the call to the method is correct.
if (tag_func[tag] != NULL) | ||
(*tag_func[tag])(len); | ||
else | ||
(*tag_func4[tag])(len, 0, partial, hash); // subsquent packets. |
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.
Idem.
The clang compiler was complaining with the error message: "Passing arguments to a function without a prototype is deprecated in all versions of C". Fix: openwall#5268. Signed-off-by: Claudio André <dev@claudioandre.slmail.me>
I created a file to use the symmetric "stuff":
This first “else” was executed once. The second was called 6 times. The same output was produced and it is crackable. |
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.
Let's figure out the discrepancy in pointer placement/counts.
NULL, | ||
NULL, | ||
NULL, | ||
Symmetrically_Encrypted_and_MDC_Packet, |
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.
The functions removed from the old/first array are separated by 8 other pointers, but here they're separated by only 7 NULLs. A bug?
Separately (but maybe related), I notice that the old array initializer had 65 lines, whereas the tag mask is such that there should probably be only 64 - a bug? Harmless?
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.
That's cool. No idea how it's possible if I counted the pointers between the two 4-argument functions right. Was the code buggy prior to your changes? Did gpg2john produce the same or different output before this PR's changes? Anyhow, please add your new test input to the |
I'll push (soon) it again to see if anything is missing! |
6614733
to
79e8b91
Compare
@claudioandre-br I see you've inserted the missing NULL now. However, that this bug went unnoticed means we have no test case for this functionality (the function that was presumably at the wrong index before this NULL addition), right? |
My local version has been tested ('íf' and 'else'). But let's keep this waiting until i can test it again. There weren't supposed to be two different versions. |
This is the test that was executed: From d6b22c64d2bc746aca2ff2911203ccfc6ebc27b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Claudio=20Andr=C3=A9?= <dev@claudioandre.slmail.me>
Date: Tue, 17 Dec 2024 11:30:25 -0300
Subject: [PATCH] Test gpg2john
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Signed-off-by: Claudio André <dev@claudioandre.slmail.me>
---
src/gpg2john.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/gpg2john.c b/src/gpg2john.c
index 46a3a51eb..ac1c0104f 100644
--- a/src/gpg2john.c
+++ b/src/gpg2john.c
@@ -310,6 +310,8 @@ static int mflag;
//static int pflag;
static int uflag;
+static int first1, first4, partial1, partial4;
+
public void
warning(const string fmt, ...)
{
@@ -435,6 +437,8 @@ int gpg2john(int argc, char **argv)
continue;
}
MEM_FREE(hash);
+ fprintf(stderr, "=> 1st IF %d, 1st ELSE %d, 2nd IF %d, 2nd ELSE %d\n", first1, first4, partial1, partial4);
+ first1 = first4 = partial1 = partial4 = 0;
}
if (something_failed)
@@ -1699,9 +1703,9 @@ parse_packet(char *hash)
if (gpg_dbg)
fprintf(stderr, "Packet type %d, len %d at offset %d (Processing) (pkt-type %s) (Partial %s)\n", tag, len, offset, pkt_type(tag), partial?"yes":"no");
if (tag_func[tag] != NULL)
- (*tag_func[tag])(len);
+ {(*tag_func[tag])(len);first1++;}
else
- (*tag_func4[tag])(len, 1, partial, hash); // first packet (possibly only one if partial is false).
+ {(*tag_func4[tag])(len, 1, partial, hash);first4++;} // first packet (possibly only one if partial is false).
} else {
if (gpg_dbg)
fprintf(stderr, "Packet type %d, len %d at offset %d (Skipping) (Partial %s)\n", tag, len, offset, partial?"yes":"no");
@@ -1724,9 +1728,9 @@ parse_packet(char *hash)
if (gpg_dbg)
fprintf(stderr, "Packet type %d, len %d at offset %d (Processing) (pkt-type %s) (Partial %s)\n", tag, len, offset, pkt_type(tag), partial?"yes":"no");
if (tag_func[tag] != NULL)
- (*tag_func[tag])(len);
+ {(*tag_func[tag])(len);partial1++;}
else
- (*tag_func4[tag])(len, 0, partial, hash); // subsquent packets.
+ {(*tag_func4[tag])(len, 0, partial, hash); partial4++; }// subsquent packets.
} else
skip(len);
}
--
2.43.0 The log of the “new sample” (the one I mentioned above). Both 'elses' were "covered". File GPG/old.hash.asc
=> 1st IF 1, 1st ELSE 1, 2nd IF 0, 2nd ELSE 6 Full log: $ cd ..
run/gpg2john GPG/*.asc GPG/gpg-70_flavors/*.sec > 2.hash
cat 2.hash | LANG=C sort > pr.hash
cd -
File GPG/old.hash.asc
=> 1st IF 1, 1st ELSE 1, 2nd IF 0, 2nd ELSE 6
File GPG/test.utf8.asc
=> 1st IF 20, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/MD5-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/RIPEMD160-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA1-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA224-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA256-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA384-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-3DES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-AES.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-AES192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-AES256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-BLOWFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-CAMELLIA128.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-CAMELLIA192.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-CAMELLIA256.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-CAST5.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
File GPG/gpg-70_flavors/SHA512-TWOFISH.sec
=> 1st IF 5, 1st ELSE 0, 2nd IF 0, 2nd ELSE 0
/host/src |
The diff (between local and PR) is just the debug patch. $ git diff origin/fix/clang fix/clang > ~/diff.txt |
Thank you, @claudioandre-br! I assume you imply this is ready to merge?
Are you yet to submit this to the |
We need to think about more/better tests (if there are other use cases). I'm not the best person to do this. I will submit the example in question and hope that people who know GPG will offer other examples. |
The clang compiler was complaining with the error message: "Passing arguments to a function without a prototype is deprecated in all versions of C".
It looks correct to me, so I'm sending it.