Fix of the bug with long Oracle passwords and creation of oracle2john.py #5643
Conversation
|
Thanks @k4amos. Can you please separate the two changes here (added tool vs. patched code) into separate commits? Also, I am really not sure the trivial fix I suggested is correct. This needs further analysis. I wouldn't be surprised if we see ASan failures after you fix the syntax error (add missing comma after the added test vector). But I don't mind you giving this a try. |
|
Oh, also we have a convention to prefix commit titles with subsystem names, e.g. @exploide As usual, we'd appreciate your review of the Python script. Thank you! (And we'll need to remember to credit you prominently for all the Python work when we make the next numbered release.) |
k4amos
force-pushed
the
bleeding-jumbo
branch
from
c718a52 to
f3714f1
Compare
k4amos
force-pushed
the
bleeding-jumbo
branch
from
f3714f1 to
9f14814
Compare
There was a problem hiding this comment.
Just added a few comments which concern mostly possible minor improvements.
However, I have two major things for discussion.
-
There already is
pcap2john.pywhich iterates through pcap files and outputs various hashes it can extract. In principle, o5logon support could have been added as a new function withinpcap2john.py. But I see that there is something different, i.e. o5logon seems to assemble information contained within multiple different packets, if I understand the code correctly? Most (but not all) functions inpcap2john.pyprocess single packet authentications, hence they don't need to keep state when parsing the next packet. So it might be fine to have this as a separate script? -
I think the script is not very robust when parsing larger captures which contain more than just the o5logon procedure. Especially the regex searching within the raw data might be prone to false positives and negatives. Imagine a plain HTTP packet where the word PBKDF2 is contained on a web page. The script would immediately exit in its current form. Is o5logon always contained in TNS packets? I don't see scapy having support for this as a layer which would be nice for filter reasons. If we can't find a way to filter relevant packets in a better way, we may at least need to document that the pcap file should only contain the o5logon/TNS stuff for best results.
| import scapy.all as scapy | ||
| except ImportError: | ||
| print( | ||
| "\033[91m[Error] Scapy seems to be missing, run 'pip install --user scapy' to install it\033[0m" |
There was a problem hiding this comment.
This should go to stderr, i.e. add the parameter file=sys.stderr to print.
|
|
||
| if auth_data["server_auth_sesskey"] is None or auth_data["auth_vfr_data"] is None: | ||
| print( | ||
| f"\033[91m[Error] The PCAP file {filename} does not contain the required fields of o5logon authentification\033[0m" |
There was a problem hiding this comment.
Should also go to stderr to avoid having this in the hash output file when redirection is used. I would also consider this being a warning (or omit it altogether) because it might be a valid use case to throw this script on a bunch of pcap files containing arbitrary network traffic.
| f"\033[91m[Error] The PCAP file {filename} does not contain the required fields of o5logon authentification\033[0m" | ||
| ) | ||
| elif None not in list(auth_data.values()): | ||
| # Format of the hash : $o5logon$ <server's AUTH_SESSKEY> * <AUTH_VFR_DATA> * <AUTH_PASSWORD> * <client's AUTH_SESSKEY> |
There was a problem hiding this comment.
I'm not familiar with the o5logon protocol and don't know why the client values can be missing. Would it make sense to add a comment explaining the two cases?
| pbkdf2_match = re.search(rb"PBKDF2", raw_data) | ||
| if pbkdf2_match: | ||
| print( | ||
| "\033[91m[Error] Sorry, PBKDF2 is not (yet) supported by this utility (only o5logon is)\033[0m" |
There was a problem hiding this comment.
stderr again. But I'm wondering, it is that fatal that the program should exit? What if another connection is contained within the pcap? And the script seems to support multiple files as well. If the first cannot be processed, the latter will never be tried?
| "([A-Fa-f0-9]+)", raw_string.decode("ascii", errors="ignore").replace(" ", "") | ||
| ) | ||
| if match_hexa: | ||
| return match_hexa.group(1) |
There was a problem hiding this comment.
explicitly return None otherwise to make the intention clear
| """, | ||
| ) | ||
|
|
||
| parser.add_argument("-f", "--file", type=str, required=True, nargs="+") |
There was a problem hiding this comment.
Since files are a mandatory and the only parameter the script expects, I would personally omit the flag-style option and just make it a positional argument instead. Most other *2john scripts also do it this way (but not all), so this is a matter of preferences.
| args = vars(parsed_args) | ||
|
|
||
| for filename in args["file"]: | ||
| auth_data = read_file(args, filename) |
There was a problem hiding this comment.
read_file does not return any auth_data, it looks rather like a process_file, prints the hash directly and returns nothing.
|
One thing I forgot to note - this is significant enough addition that we should have a |
#5610