Chordian/deepsid

Public API

tobozo opened this issue · 19 comments

tobozo commented

Hi and thanks for that awesome platform 👍

I've been developing an HVSC-based SID Player for ESP32, it acts as the C64 and is designed to drive any type of SID Chip (preferably clones).

esp32-SID+hat.mp4

Now that everything appears to work fine with the filesystem, I would like to test with an online database, and the deepsid site appears to provide an excellent API although it seems some measures are in place.

image

I know this can be easily prevented by sending the extra headers, but before doing that I'd like to ask if using the site in such a way is acceptable.

Also I found that not all queries using $_GET are prepared in hvsc.php, this smells like SQL Injection waiting to happen.

Chordian commented
tobozo commented

Hi Jens, thanks for your quick reply

Actually I'm not planning to host/deploy, the idea is to access directly this URL by mocking XHR from my ESP32.

I figured out this isn't really a public API and no auth token is required, but since I found some limitating measures in the code I'm asking for a permission before starting to implement the client logic on my ESP32 and send queries to the actual deepsid.chordian.net website.

re: SQLI

php/hvsc.php:81

$compoName = $isCSDbCompo && strlen($_GET['folder']) > 25 ? explode('/', $_GET['folder'])[2] : '';

this $compoName variable isn't filtered and is used with unprepared query() statements.

Chordian commented
Chordian commented
tobozo commented

whoops I was merely providing this info as a hint after quick-reading the code in just one file, but a slower read of hvsc.php brought up some other points of interest:

  • possible resource exposure here
  • possible stored XSS here
  • these code blocks: [1] [2] [3] look suspicious and may contain more SQL Injections

I was too lazy to read the other files so I ran an audit tool (phpvuln) which found 346 potential vulns (sql injections, remote file inclusions, XSS, and a rand key).

There's no reason to panic about these numbers as many detected vulns are either false positives or impossible to reach, but reducing them would provide a better view on the true positives.

Chordian commented
Chordian commented
tobozo commented

Hi Jens,

I'm assuming you have already enabled Varnish WAF Core Rule Set so I'll skip the server part and focus on the code part.

You're probably looking for validation and sanitizing filters.

Doing something like this is tempting, but it is wrong for several reasons:

$clean_get = array_filter($_GET, function($k, $v) {
    return filter_var( $v, FILTER_VALIDATE_REGEXP, $regexp_filter_opt );
}, ARRAY_FILTER_USE_BOTH);
  • a higher security level can't achieved by just factorizing, it needs deconstructing first
  • not all _GET variables need filtering, some can be just matched with known clean values e.g. in_array()
  • a single _GET variable may have several escaping strategies depending on the output contexts

Sanitizing example:

$search_html = filter_input(INPUT_GET, 'search', FILTER_SANITIZE_SPECIAL_CHARS);
$search_url = filter_input(INPUT_GET, 'search', FILTER_SANITIZE_ENCODED);
echo "You have searched for $search_html.\n";
echo "<a href='?search=$search_url'>Search again.</a>";

// will print:
// You have searched for Me &#38; son.
// <a href='?search=Me%20%26%20son'>Search again.</a>

Validating (and halting if validation fails):

$my_regexp = "/^[a-z0-9-_]{3,}$/"; // only-accept_words and numbers, no spaces, no special chars, strlen>=3
$regexp_filter_opt = ["options" => ["regexp"=> $my_regexp ] ];
$my_var = filter_var( $_GET['blah'], FILTER_VALIDATE_REGEXP,  $regexp_filter_opt  ) or die("Bad input, halting");

Depending on the situation you should either match, validate, sanitize, or validate+sanitize.

Phpvuln can help you establish an inventory of those variables and create a table where you check boxes depending of what the variable does, if it needs validation, and what to do if the validation fails.

file var name html database filesystem sanitize validate on fail
blah.php $_GET['blah'] ✔ ✔ ✔ ✔ ✔ 💀
index.php $_GET['foo'] ✖ ✔ ✖ ✔ ✔ 💀
includes/something.php $_GET['bar'] ✔ ✖ ✖ ✔ ✖ 🤷
vars.php $folderPath ✖ ✖ ✔ ✖ ✔ 💀

Building such a list produces many virtuous effects:

  • it can be used to produce enforced WAF rules
  • it helps you abstract away validation/sanitizing (e.g. using either php native filters or Laravel Validation class)
  • items can be sorted/grouped by concerns to help define each security perimeter and reduce the logic overhead
  • just add a "progress" column and you have a roadmap
Chordian commented
tobozo commented

welp I initially came here to ask permission to access the hvsc.php "API" from my ESP32-SIDPlayer's http client so it can retrieve playlists and SID files 🤣

the fact that my ESP32 can do it probably means the access level to hvsc.php is "public" so I'll assume it's okay, but I'm currently coding a full client without knowing if this access level is going to change in the future, and knowing so would considerably reduce my own tech debt.

happy end of year!

Chordian commented
  • 🎉1
tobozo commented

happy $7e7 🥳

here's some progress on my deepsid client for esp32:

📱 https://youtube.com/shorts/maYoC9-8GKM
💻 https://youtu.be/maYoC9-8GKM

Chordian commented
tobozo commented

btw I noticed an issue with the player selector: when WebSid emulator is selected but WASM is disabled in the browser: a javascript error prevents the player selector to be rendered, and the whole deepsid app becomes unusable.

image

The problem is that the default player WebSid emulator is a WASM app; as a consequence any new visitor with WASM disabled will end up on a broken Deepsid site.

image

Detecting WASM support and using the JavaScript player as a fallback when detection fails is probably the best strategy to prevent this broken state.

image

Some context: recent news about possible usage of WASM by malware writers have increased an already existing awareness.

Chordian commented
tobozo commented

I guess people do that when they don't like the idea that a full linux system could run in their browser.

Kroc commented

May be disabled by default with certain ad-blocking or privacy extensions. You should at least fail gracefully so that users know to allow permissions

  • 👍1
Chordian commented
Chordian commented

DeepSID now shows a message if WASM is disabled.