Dieser Beitrag gilt einzig und allein dem Ziel der Qualitätssteigerung. Ich bin kein Spezialist - und deshalb erschüttert es mich, dass ich mich in der Lage sehe, in offenen Quelltexten von INSTAR Fehler und Sicherheitslücken zu entdecken. Ich würde dieses Code-Review auch nicht schreiben, wenn die INSTAR-Produkte nicht den Sicherheitssektor beträfen. Das ist der Sektor, in dem es so sehr darauf ankommt, pingelig und pedantisch zu sein!
Ich betrachte hier den von INSTAR im Download-Bereich veröffentlichten WebSocket-Client (html_ws_client.zip) für die Einbindung eines Livestreams in eigene Webseiten. Wenn ich diesen Code lese, mag ich gar nicht daran denken, dass das Backend möglicherweise genauso schnell und unbedacht entwickelt wird; dieser Quelltext eignet sich hervorragend, um Studierenden aufzuzeigen, was man alles falsch machen kann. Wenn ich mir den Code anschaue, habe ich das Gefühl, das INSTAR keinerlei Qualitätskriterien für den entwickelten Code festlegt. Im Gegenteil: der Code sieht aus, als wäre er in einem Wahnsinns-Tempo geschrieben, was ihn leider unwahrscheinlich schlecht macht. Insbesondere kann ich mir nicht vorstellen, dass hier überhaupt mehr als ein Paar Augen an der Entwicklung beteiligt war. Noch einmal zur Sicherheit: Ich will niemanden persönlich angreifen oder gar verletzen, sondern mache mir diese Mühe nur, um zu helfen.
Grundsätzlich: Wenn man heute JavaScript schreibt, sollte man var vermeiden und statt dessen const verwenden, wo möglich, und nur dort let verwenden, wo nötig.
index.html: Zeile 35:
<script src="wsclient.js"></script>
wsclient.js: Zeilen 1-7:
// Add your camera configuration here
var ws_protocol = "ws";
var ws_hostname = "192.168.2.120";
var ws_port = "80";
var ws_endpoint = "/ws";
var cam_username = "admin";
var cam_password = "instar";
Wieso werden äußerst sensible Daten auf globaler Ebene abgelegt? Das ist ein Sicherheitsrisiko! Sobald INSTAR-Kunden weitere Skripte über script-Tags einbinden, können die Zugangsdaten an beliebige Server versendet werden - und nein, die Same-Origin-Policy unserer Webbrowser schützt uns nicht, denn es genügt ein dynamisch erzeugtes img-Tag mit einem src-Attribut à la https://uglyserver.example.com/?STOLEN_DATA - und schon sind die Daten erfolgreich abgefischt! Einfache Methoden, um diesem Problem entgegenzuwirken, sollten lokale Namensräume (oder insbesondere Module) sein - aber auch das will wohlüberlegt sein!
wsclient.js: Zeilen 43-51:
if (codec_mime.value = 0) {
var mimeCodec = 'video/mp4; codecs="avc1.4D001E, mp4a.40.2"';
} else if (codec_mime.value = 1) {
var mimeCodec = 'video/mp4; codecs="avc1.4D001E"';
} else if (codec_mime.value = 2) {
var mimeCodec = 'video/mp4; codecs="hev1.2.4.L120.B0, mp4a.40.2"';
} else {
var mimeCodec = 'video/mp4; codecs="hev1.2.4.L120.B0"';
}
Die Variable codec_mime ist offenbar vorgesehen, um ganze Zahlen zu speichern. Warum wird hier codec_mime.value angeschaut? Das ergibt keinen Sinn. Davon abgesehen suggerieren diese Zeilen, dass der Wert von codec_mime.value überprüft würde, was leider nicht passiert. Da die Bedingungen hier Zuweisungen (nur ein Gleichheitszeichen statt zweien! Das ist ein Fehler, der darauf hinweist, dass der Code überhaupt nicht getestet wurde!) sind, ist der initiale Wert von codec_mime.value gänzlich unerheblich; nach Zeile 51 gilt immer, dass mimeCodec == 'video/mp4; codecs="avc1.4D001E"'. Der Benutzer kann hier also effektiv gar nichts auswählen!
wsclient.js: Zeile 106:
if ((ws_protocol === '') || (ws_hostname === '') || (ws_port === '') || (ws_endpoint === '') ||(cam_username === '') || (cam_password === '')) {
Was soll diese Überprüfung nutzen? Das Backend sollte (hoffentlich!) nicht darauf angewiesen sein, dass diese Zeichenketten alle nicht-leer sind. Wenn sie es nicht sind, kommt eben keine Verbindung zustande. Weg damit - schlankerer Code ist besserer Code!
wsclient.js: Zeile 126:
webSocketURL = protocol + "://" + username + ":" + password + "@" + hostname + ":" + port + endpoint;
So kann man die webSocketURL beim besten Willen nicht konstruieren. Wenn username und/oder password Sonderzeichen enthalten, gibt es schnell Schwierigkeiten: einerseits wird die Verbindung nicht funktionieren - andererseits werden ggf. Teile der Zugangsdaten als hostname interpretiert, was wiederum unmittelbar eine Sicherheitslücke bedeutet! Sowohl username als auch password müssen hier unbedingt mit encodeURIComponent() kodiert werden, bevor sie in die webSocketURL eingebaut werden!
Eine Start-Stop-Automatik ist in meinen Augen insgesamt wenig sinnvoll. Besser wäre ein automatisch startender Viewer. Warum liefert die Kamera einen WebSocket-Viewer nicht passwortgeschützt über ihren HTTP-Server aus? Mit Hilfe von Reverse-Proxys kann man die auch elegant und sicher nach „außen“ weiterleiten, wenn man das möchte.
Abschließend hoffe ich sehr, dass INSTAR hier die Größe besitzt, um Einsicht zu zeigen. Quelltexte wie diese sind einfach katastrophal und sollten nicht an Endkunden ausgeliefert werden (die häufig von Technik keine Ahnung haben und diese unverändert einsetzen!). Ein guter Vorsatz für das neue Jahr sollte sein, dass INSTAR alle Quelltexte (etwa auf GitHub) offenlegt. Niemand kauft INSTAR-Kameras, weil die Software so einzigartig und unnachahmlich wäre, sondern ganz einfach, weil man auf die Hardware angewiesen ist - von der ich sehr viel halte! Eine Quellcode-Offenlegung wird sicher viele Sicherheitslücken offenbaren - und das ist großartig, weil sie dann geschlossen werden können!