Cross-site scripting vulnerabilities in PHP

[ha5dzs]

Hi,

Our IT department regularly scans all public servers for vulnerabilities. Normally, these are some AI-based tools that look for missed updates or bad configurations.

This time, they found a problem, and I managed to replicate it.

So in views/search.php, you directly output the imperialUnits HTTP POST variable into the html document that is being sent back to the client.

Now, normally, this should only be a boolean, but I can literally put anything into it, and it will be in the html document as if it was part of the server's code.

I fixed this by adding a simple sanity check. I declared this into a boolean, where the value is 0 when it is set to 0 or 1 if anything else is set.

So, no matter what you supply as the value of imperialUnits, it will always be either 0 or 1 in the html document.

I would create a pull request, but I kind of butchered your code to fit our needs and it no longer can be merged.

See the commit here.

Essentially I did:

$use_imperial_units = ($_GET['imperialUnits'] != 0) ? 1 : 0;

...and further down, instead of:

 <a class="tdlink" href="/views/overview.php?id=<?php echo $foundStation->id; ?>&imperialUnits=<?php echo $_GET['imperialUnits'] ?? 0; ?>"><?php echo htmlentities($foundStation->name) ?></a>

.
.
.

 loadView('/views/search.php?imperialUnits=<?php echo $_GET['imperialUnits'] ?? '0'; ?>&q=' + q + '&seconds=' + seconds);

I did

 <a class="tdlink" href="/views/overview.php?id=<?php echo $foundStation->id; ?>&imperialUnits=<?php echo $use_imperial_units ?>"><?php echo htmlentities($foundStation->name) ?></a>

.
.
.

loadView('/views/search.php?imperialUnits=<?php echo $use_imperial_units ?>&q=' + q + '&seconds=' + seconds);

I did a brief test and now it behaves as it should. While this is a fairly minor problem (you'll need to click on a 'poisoned' link in order for a third-party script to be run), it will probably be flagged up as an issue for all implementations eventually.

(edit: 'sent back to the client', and I found some typos)

[ha5dzs]

Just to let you know, the same stuff is present in overview.php, statistics.php, trail.php, weather.php, and telemetry.php. There are also no sanity checks on the category variable in raw.php.

In addition, there are basically no sanity checks on the sid (station ID) variable in index.php, search.php and latest.php.

In index.php, the center variable, and on the q (search query) variables also do not have any sanity checks.

The code is now too different for me to be able to submit a pull request, but in a nutshell:

• I just set the imperialUnits variable as a boolean as per the description
• I cast category and sid as integers, so if anyone adds some naughty code, it will just break the page
• I limited the lengths of center and q. This is probably a bit dirty, because theoretically the precision of GPS coordinates can be high and one could submit many callsigns as search query. But, in NYUAD APRS, precision is limited, and hardly anyone uses the search feature, so I might be able to get away with it :)
• Additionally, I also disabled some of the filtering too in index.php lines 135-139.

Hope this helps.