Closed m1cm1c closed 8 months ago
Die ideale Lösung ist, dass alle Probleme sofort mit einem Aufruf gelöst werden und die Lösung funktioniert.
Für die 2 Beispiele sieht das wie folgt aus:
Mögliche Zwischenschritte:
Gehen deine Gedanken in eine ähnliche Richtung?
Die ideale Lösung ist, dass alle Probleme sofort mit einem Anruf gelöst werden
Aber nur in einem speziellen Modus für Änderungen, nicht im Analyse-Modus, oder?
Selbst da stimme ich nicht ganz zu, dass das die optimale Lösung ist: Wenn die Git History neu geschrieben wird, muss das mit allen anderen Entwicklern koordiniert werden. Es kann also Zeit dauern, bis das möglich ist, wohingegen andere Änderungen sofort möglich sind und nicht auf Koordination mit anderen warten müssen sollte. Außerdem können wir gar nicht immer wissen, was die beste Lösung ist. Z.B. kann es sein, dass bzgl einem Geheimnis das Kind schon in den Brunnen gefallen ist (Repo ist schon öffentlich, ggf existieren sogar schon Forks; dann kommt nur der Austausch des Geheimnisses in Betracht).
Aber nur in einem speziellen Modus für Änderungen, nicht im Analyse-Modus, oder?
Ja den Änderungsmodus muss man explizit aktivieren, entweder durch ein flag -fix oder einen dedizierten command.
Bzgl. der Problematik mit der Historie:
Dann unterscheiden wird doch zunächst mal die bei Modi (Definition gemäß https://github.com/secguro/secguro-cli/issues/67)
"verwendung einer funktion mit sicherheitsproblemen" => wie oben; zusatz: wenn es mehrere lösungen gibt, wird die "beste" ausgewählt; im interaktiven anwendungsmodus (sollte es diesen geben), können auch mehrere lösungen angezeigt und darunter ausgewählt werden.
"gefundenes eingechecktes Geheimnis" => prinzipiell kann ein Geheimnis im Code vorhanden sein, wurde aber noch nicht eingecheckt, sollte dann aber trotzdem entfernt werden => ansatz wie oben
"verwendung einer funktion mit sicherheitsproblemen" => n/a
"gefundenes eingechecktes Geheimnis" => wie oben; zusatz: wenn mind. ein geheimnis gefunden und ersetzt wurde, wird am Ende immer der Hinweistext angezeigt (sinngemäß): "Ihr Geheimnis kann bereits in falsche Hände geraten sein. Sie sollten dieses schnellstmöglichst ändern"
ich habe betrachtet, welche weiteren probleme automatisiert behoben werden können, und dafür sämtliche findings auf dem wallet-projekt betrachtet
bzgl gitleaks gab es erwartungsgemäß keine weiteren problemarten
bzgl semgrep gab es die die folgenden problemarten (und natürlich solche wie generic.secrets.security.detected-generic-api-key.detected-generic-api-key
und generic.secrets.security.detected-jwt-token.detected-jwt-token
, die analog zu gitleaks sind; außerdem hab ich alle finding-arten, bei denen ein beheben offensichtlich nicht automatisiert möglich ist, weggelassen):
Finding 7:
detector: semgrep
rule: yaml.docker-compose.security.no-new-privileges.no-new-privileges
file: docker-compose.dev-ganache.yml
line: 4
column: 3
match: localchain1:
hint: Service 'localchain1' allows for privilege escalation via setuid or setgid binaries. Add 'no-new-privileges:true' in 'security_opt' to prevent this.
commit hash: 0f8f5e0ceed6bc6f08c3b1b128899942f0dcfdaa
commit date: 2023-04-04T08:35:46Z
author: Matthias Stumpp
author email address: [zensiert]
commit summary: Added 2 ganache chains
und
Finding 45:
detector: semgrep
rule: yaml.kubernetes.security.allow-privilege-escalation-no-securitycontext.allow-privilege-escalation-no-securitycontext
file: server/manifests/deployment_prod.yml
line: 19
column: 11
match: - name: wallet-server
hint: In Kubernetes, each pod runs in its own isolated environment with its own set of security policies. However, certain container images may contain `setuid` or `setgid` binaries that could allow an attacker to perform privilege escalation and gain access to sensitive resources. To mitigate this risk, it's recommended to add a `securityContext` to the container in the pod, with the parameter `allowPrivilegeEscalation` set to `false`. This will prevent the container from running any privileged processes and limit the impact of any potential attacks. By adding a `securityContext` to your Kubernetes pod, you can help to ensure that your containerized applications are more secure and less vulnerable to privilege escalation attacks.
commit hash: 2920d72106ee98bab7a77daad7a3fc7b0a17b7d7
commit date: 2022-03-19T10:12:29Z
author: Matthias Stumpp
author email address: [zensiert]
commit summary: Added manifests for other stages
lässt sich trivial gemäß dem hinweis beheben => automatisiertes beheben möglich
Finding 8:
detector: semgrep
rule: yaml.docker-compose.security.writable-filesystem-service.writable-filesystem-service
file: docker-compose.dev-ganache.yml
line: 4
column: 3
match: localchain1:
hint: Service 'localchain1' is running with a writable root filesystem. This may allow malicious applications to download and run additional payloads, or modify container files. If an application inside a container has to save something temporarily consider using a tmpfs. Add 'read_only: true' to this service to prevent this.
commit hash: 0f8f5e0ceed6bc6f08c3b1b128899942f0dcfdaa
commit date: 2023-04-04T08:35:46Z
author: Matthias Stumpp
author email address: [zensiert]
commit summary: Added 2 ganache chains
lässt sich trivial gemäß dem hinweis beheben => automatisiertes beheben möglich, aber der nutzer muss darauf hingewiesen werden, dass vermutlich anpassungen an seiner anwendung notwendig sind, die wir nicht automatisiert durchführen können
Finding 11:
detector: semgrep
rule: javascript.lang.security.detect-insecure-websocket.detect-insecure-websocket
file: docker-compose.dev-ganache.yml
line: 41
column: 34
match: # ETHEREUM_JSONRPC_WS_URL: ws://localchain1:8545
hint: Insecure WebSocket Detected. WebSocket Secure (wss) should be used for all WebSocket connections.
commit hash: 33bc659f707532e7fc741dc3b40ad405cf4d7d8e
commit date: 2023-04-07T22:19:30Z
author: Matthias Stumpp
author email address: [zensiert]
commit summary: Disabled block explorers
beheben ist an sich automatisiert möglich, aber vermutlich oftmals nicht sinnvoll, weil eine verschlüsselte verbindung manchmal nicht möglich ist oder keinen/kaum mehrwert liefert (z.B. im gezeigten beispiel)
Finding 37:
detector: semgrep
rule: yaml.kubernetes.security.run-as-non-root.run-as-non-root
file: documentation/docusaurus/manifests/deployment_cd.yml
line: 17
column: 5
match: spec:
hint: When running containers in Kubernetes, it's important to ensure that they are properly secured to prevent privilege escalation attacks. One potential vulnerability is when a container is allowed to run applications as the root user, which could allow an attacker to gain access to sensitive resources. To mitigate this risk, it's recommended to add a `securityContext` to the container, with the parameter `runAsNonRoot` set to `true`. This will ensure that the container runs as a non-root user, limiting the damage that could be caused by any potential attacks. By adding a `securityContext` to the container in your Kubernetes pod, you can help to ensure that your containerized applications are more secure and less vulnerable to privilege escalation attacks.
commit hash: a2f9b797ee6bc1e07dfa9328bfa88ef8647a8c06
commit date: 2021-11-22T13:29:01Z
author: Christoph Michelbach
author email address: [zensiert]
commit summary: Document the wallet's features.
an sich trivial zu fixen, aber der fix scheint oft nicht sinnvoll zu sein: https://stackoverflow.com/a/51544669 außerdem bringt er ggf probleme mit sich. nicht nur, wenn tatsächlich operationen mit root-rechten durchgeführt werden müssen, sondern auch, wenn einfach kein anderer user existiert
Finding 52:
detector: semgrep
rule: javascript.lang.security.audit.detect-non-literal-regexp.detect-non-literal-regexp
file: server/src/helpers/email.ts
line: 33
column: 19
match: const regex = new RegExp(`\\\${${key}}`, 'g');
hint: RegExp() called with a `{content, variables}: TemplateEngineProps` function argument, this might allow an attacker to cause a Regular Expression Denial-of-Service (ReDoS) within your application as RegExP blocks the main thread. For this reason, it is recommended to use hardcoded regexes instead. If your regex is run on user-controlled input, consider performing input validation or use a regex checking/sanitization library such as https://www.npmjs.com/package/recheck to verify that the regex does not appear vulnerable to ReDoS.
commit hash: 2ca6beaf16cbe8e9c35d80fe7e7100225e86343e
commit date: 2022-02-03T12:51:08Z
author: Matthias Stumpp
author email address: [zensiert]
commit summary: Added initial version of email send
lässt sich nicht automatisiert beheben, und vermutlich überhaupt nicht, außer in fällen, in denen es ein false positive ist
Finding 53:
detector: semgrep
rule: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal
file: server/src/helpers/email.ts
line: 80
column: 5
match: getFileName(view, locals)
hint: Detected possible user input going into a `path.join` or `path.resolve` function. This could possibly lead to a path traversal vulnerability, where the attacker can access arbitrary files stored in the file system. Instead, be sure to sanitize or validate user input first.
commit hash: 2ca6beaf16cbe8e9c35d80fe7e7100225e86343e
commit date: 2022-02-03T12:51:08Z
author: Matthias Stumpp
author email address: [zensiert]
commit summary: Added initial version of email send
beheben scheint mir nicht automatisiert möglich zu sein. statt vom file system zu lesen, sollte der nutzer vermutlich in den meisten fällen eine DB einsetzen
Finding 65:
detector: semgrep
rule: typescript.react.security.audit.react-jwt-decoded-property.react-jwt-decoded-property
file: webapp/src/App.tsx
line: 116
column: 10
match: if(decoded_token.exp === undefined)
hint: Property decoded from JWT token without verifying and cannot be trustworthy.
commit hash: cba2cfd872ac0002a7366d6b2cddc4801a9e9068
commit date: 2022-09-20T09:58:23Z
author: Christoph Michelbach
author email address: [zensiert]
commit summary: Fix linting violations and discovered bugs and reduce the number of acceptable linting violations.
kann vermutlich nicht automatisiert gefixt werden, weil wir nicht wissen können, wie das JWT verifiziert werden kann (z.B. wo das geheimnis liegt)
als automatisiertes beheben können wir ein upgrade auf die nächste version, die nicht von der sicherheitslücke betroffen ist, anbieten (sofern vorhanden). wenn man auf die nächste version statt auf die aktuell neueste wechselt, sollte in den meisten fälle kompatabilität bestehen
stimmst du meiner beurteilung zu?
Gitleaks
stimme zu
Sicherheitslücken
Ich stimme zu, dass einige Fehler leichter automatisiert behebbar sind als andere. Einige Fehler sind wahrscheinlich überhaupt nicht automatisiert behebbar (z.B. Fehler, die mehrere Komponenten betreffen).
Zu den automatisiert behebbaren Fehlern: Ich sehe nicht, wie man den Fehler einfach so aus dem Hint heraus beheben kann. Für eine automatisierte Fehlerbehebung braucht man entweder eine maschinenverarbeitbare Lösungsbeschreibung (was bei den obigen Beispielen nicht der Fall ist) oder eben den ChatGPT-Ansatz (ich vermute aber, dass du daran nicht gedacht hast, als du "automatisierte Behebung möglich" geschrieben hast).
Was wir auf keinen Fall machen, ist für jede SemGrep-Regel eine maschinenverarbeitbare Lösungsbeschreibung zu definieren oder einen Parser zu bauen, der den in natürlicher Sprache geschriebenen Hint analysiert und in eine Lösung übersetzt.
Dependency-Check
Ich bin noch unschlüssig, wie man hier idealerweise vorgehen sollte.
Ich sehe zumindest folgende Optionen: a. Auf die nächste version upgraden, für die keine Sicherheitslücke bekannt ist b. Upgrade auf die neueste Version, sofern keine Sicherheitslücke bekannt ist (der Dependabot aktualisiert doch auch auf die neueste Version?), ansonsten auf eine ältere Version, für die erstmals keine Sicherheitslücke bekannt ist (sinnvoll in Fällen, in denen der Autor nicht schnell genug einen Patch zur Verfügung stellt).
Sowohl a. als auch b. können entweder
Schlussfolgerung:
Lass uns erstmal auf die Fehlerbehebung der mit gitleaks gefundenen Secrets sowie den Dependency-Check konzentrieren.
Was wir auf keinen Fall machen, ist für jede SemGrep-Regel eine maschinenverarbeitbare Lösungsbeschreibung zu definieren
Das wäre mein Ansatz gewesen, denn ich gehe nicht davon aus, dass so viele Problemarten automatisiert behoben werden können.
D.h. beim Beheben von allen Problemen außer beim Entfernen von Geheimnissen, setzt du voll auf ChatGPT?
der Dependabot aktualisiert doch auch auf die neueste Version?
Quelle? Denn das entspricht weder meinen Erwartungen an einen Sicherheitsfix, noch was irgendein mir bekanntes System mit Paketmanager, das ich jemals verwendet habe, gemacht hat (die ja vor einer sehr ähnlichen Entscheinung stehen), noch meinen Beobachtung davon, was Dependabot macht, noch dem was Dependabot über sich selbst sagt:
"Dependabot checks whether it's possible to upgrade the vulnerable dependency to a fixed version without disrupting the dependency graph for the repository. Then Dependabot raises a pull request to update the dependency to the minimum version that includes the patch and links the pull request to the Dependabot alert, or reports an error on the alert." (https://docs.github.com/en/code-security/dependabot/dependabot-security-updates/about-dependabot-security-updates)
interaktiv anwenden (one-by-one)
Ganz klar mein Favorit. Ich stelle mir das Beheben interaktiv vor.
Das wäre mein Ansatz gewesen, denn ich gehe nicht davon aus, dass so viele Problemarten automatisiert behoben werden können.
Dazu müsste man erstmal alle Regeln von Semgrep analysieren um dann mit einem geeigneten Set an Problemlösungen daherzukommen. Das ist aufwendig und am Ende ist der Lock-In in Semgrep noch größer, da die Problemlösungen womöglich nur in Verbindung mit den Semgrep Regel funktionieren. Oder hältst du es für möglich die Problemlösungen agnostisch vom Problem-Detektor (z.B. Semgrep) zu gestalten und so wiederverwendbar zu machen?
D.h. beim Beheben von allen Problemen außer beim Entfernen von Geheimnissen, setzt du voll auf ChatGPT?
Halte ich für möglich.
Quelle? Denn das entspricht weder meinen Erwartungen an einen Sicherheitsfix, noch was irgendein mir bekanntes System mit Paketmanager, das ich jemals verwendet habe, gemacht hat (die ja vor einer sehr ähnlichen Entscheinung stehen), noch meinen Beobachtung davon, was Dependabot macht, noch dem was Dependabot über sich selbst sagt:
War nur eine Annahme meinerseits. Ok, gemäß der Definition folgt dependabot Option a.
Bzgl. dependency Check machen wir Option a, also analog dependabot.
Bzgl. der Art der Anwendung one-by-one (interaktiv).
Bzgl. Semgrep Regel stellen wir vorerst zurück. Da will ich erstmal mit ChatGPT experimentieren.
Dazu müsste man erstmal alle Regeln von Semgrep analysieren um dann mit einem geeigneten Set an Problemlösungen daherzukommen.
Ich würde mich auf Regeln beschränken, die in echten Projekten True Positives liefern.
da die Problemlösungen womöglich nur in Verbindung mit den Semgrep Regel funktionieren. Oder hältst du es für möglich die Problemlösungen agnostisch vom Problem-Detektor (z.B. Semgrep) zu gestalten und so wiederverwendbar zu machen?
Ich verstehe nicht, wieso die Lösung abhängig vom Detektor, der das Problem gefunden hat, sein soll. Die Lösung ist doch rein abhängig von der Problemart und dem Ort des Problems.
Bzgl. Semgrep Regel stellen wir vorerst zurück. Da will ich erstmal mit ChatGPT experimentieren.
Wenn das zuverlässig funktioniert, wäre das natürlich sehr schön. Allerdings kann ich mir hier viel eher vorstellen, dass ein Lock-In zu Semgrep entsteht: Wir müssen ChatGPT vermutlich den von Semgrep ausgegebenen Lösungshinweis (der vermutlich urheberrechtlich geschützt ist) geben, damit es zuverlässig Lösungen finden kann.
Ich verstehe nicht, wieso die Lösung abhängig vom Detektor, der das Problem gefunden hat, sein soll. Die Lösung ist doch rein abhängig von der Problemart und dem Ort des Problems.
ok. mach doch bitte einmal ein beispiel
Wenn das zuverlässig funktioniert, wäre das natürlich sehr schön. Allerdings kann ich mir hier viel eher vorstellen, dass ein Lock-In zu Semgrep entsteht: Wir müssen ChatGPT vermutlich den von Semgrep ausgegebenen Lösungshinweis (der vermutlich urheberrechtlich geschützt ist) geben, damit es zuverlässig Lösungen finden kann.
mal unabhängig vom urheberrrecht, den hint an chatgpt zu übergeben wäre ja kein problem, da dies keinen zusätzlichen aufwand erfordert.
wie gesagt, den chatgpt weg müssen wir erst verproben bevor wir hier weiter annahmen treffen.
mach doch bitte einmal ein beispiel
wenn ich ein geheimnis finde, werde ich das problem mit derselben funktion beheben, egal ob ich darüber von semgrep über eine regel namens "generic.secrets.security.detected-generic-api-key.detected-generic-api-key" oder von gitleaks über eine regel namens "generic-api-key" gelernt habe. wenn es ein anderes tool gibt, das "yaml.docker-compose.security.no-new-privileges.no-new-privileges" (wahrscheinlich unter anderem namen) diagnostiziert, werde ich dementsprechend auch dieselbe funktion verwenden, die "yaml.docker-compose.security.no-new-privileges.no-new-privileges" durch semgrep diagnostiziert behebt
Passt für mich.
Setze das doch bitte einmal beispielhaft für eines der obigen Findings um, z.B. Finding 7, so dass man das Finding via Konsole im iterativen Anwendungsmodus beheben kann.
bzgl. dem entfernen von geheimnissen: git filter-repo
macht komische dinge (z.B. entfernt es alle remotes). daher finde ich, dass wir das nicht verwenden sollten. bfg
tut das nicht, weigert sich aber standardmäßig, den zustand des neuesten commits zu verändern. ist an sich kein problem, denn es gibt einen switch, um dieses verhalten zu ändern. auch dass wir das geheimnis durch eine von uns vorgegebene UUID oder durch einen vom nutzer eingegebenen string ersetzen, ist technisch kein problem. aber ich finde die begründung sehr überzeugend:
By default the BFG doesn't modify the contents of your latest commit on your master (or 'HEAD') branch, even though it will clean all the commits before it.
That's because your latest commit is likely to be the one that you deploy to production, and a simple deletion of a private credential or a big file is quite likely to result in broken code that no longer has the hard-coded data it expects - you need to fix that, the BFG can't do it for you. Once you've committed your changes- and your latest commit is clean with none of the undesired data in it - you can run the BFG to perform it's simple deletion operations over all your historical commits.
(https://rtyley.github.io/bfg-repo-cleaner/)
ich finde, dass das eine recht elegant lösung für mehrere probleme ist, für die wir zwar lösungen haben, von denen die lösungen aber sehr komisch sind:
bfg
anweisen, das geheimnis zu entfernen, wird es auch an stellen entfernt, die der nutzer gar nicht auf dem schirm hatte. ein geheimnis mittels bfg
nur aus bestimmten dateien zu entfernen, scheint nicht möglich zu sein. dieses problem könnten wir in sich dadurch lösen, dass wir erst mal alle von git getrackten dateien im zustand des neuesten commits auf das geheimnis scannen und dem nutzer alle pfade anzeigen, wo wir es gefunden haben, aber einfach überhaupt nicht zuzulassen, dass das geheimnis im zustand des neuesten commits vorkommt, stellt wesentlich zuverlässiger sicher, dass der nutzer dazu in der lage ist, alle vorkehrungen zu treffen, dass das projekt überall ohnen dieses geheimnis auskommtich finde daher, dass wir den ansatz mit dem ersetzen des geheimnisses durch eine UUID verwerfen sollten
punkt 6 kann etwas dadurch abgeschwächt werden, dass durch die invarianz des zustandes des neusten commits das geheimnis an den anderen stellen im neuesten commit weiterhin enthalten sein kann, aber es aus der history entfernt zu haben und es dann an weiteren stellen präsent zu haben, wirkt auf mich dennoch etwas komisch. außerdem kann es vorkommen, dass das geheimnis auf diese weise überlebt, denn ich habe schon beobachtet, dass gitleaks geheimnisse abhängig vom kontext findet: wss://adomain.io/v2/apiKey=2cd6ee2c70b0bde53fbe6cac3c8b8bb1
wird erkannt, aber wss://adomain.io/v2/2cd6ee2c70b0bde53fbe6cac3c8b8bb1
wird nicht erkannt, obwohl das geheimnis identisch ist
ok, den letzten commit fassen wir nicht an. in den vorherigen commits werden die geheimnisse einfach entfernt => blank string
Setze das doch bitte einmal beispielhaft für eines der obigen Findings um, z.B. Finding 7, so dass man das Finding via Konsole im iterativen Anwendungsmodus beheben kann.
ich habe mich erst mal weiterhin an
Lass uns erstmal auf die Fehlerbehebung der mit gitleaks gefundenen Secrets sowie den Dependency-Check konzentrieren.
gehalten, denn das entfernen von secrets aus der history wird mit chat GPT auf keinen fall funktionieren, wohingegen ich bei finding 7 hoffnung habe, dass es das hinbekommt. vielleicht brauchen wir ja überhaupt keine manuell programmierten fix-regeln, abgesehen von dem entfernen von geheimnissen aus der git history
das fixen von geheimnissen habe ich soeben implementiert (#77)
warte weiterhin auf rückmeldung zur frage in slack bzgl chat GPT
beschlossen: der nutzer bekommt ein diff ähnlich einem von git angezeigten diff angezeigt, das er bestätigen muss, bevor die änderung einer automatisierten behebung durch einen von ChatGPT vorgegebenen vorschlag angewandt wird
falls in der zukunft ein diff ohne hunks gewünscht sein sollte: github.com/sergi/go-diff/diffmatchpatch bietet das an
newContent := resp.Choices[0].Message.Content
dmp := diffmatchpatch.New()
diff := dmp.DiffMain(string(fileContents), newContent, false)
fmt.Printf("old content:")
fmt.Printf(string(fileContents))
fmt.Println("new content:")
fmt.Println(newContent)
fmt.Println("diff:")
fmt.Println(dmp.DiffPrettyText(diff))
Wie sieht das aus? Url => 404
das war keine frage, nur eine notiz für mich später, damit ich nicht doppelt diff-tools recherchieren muss ;)
die URL funktioniert, nur nicht in deinem browser ;) das ist eine go-abhängigkeits-url, keine web-url. die entsprechende web-url ist: https://github.com/sergi/go-diff/tree/master/diffmatchpatch
das tool hat verschiedene darstellungsformen. diese hier halte ich für am sinnvollsten:
in jedem fall kürzt einem das tool aber selbst lange gleichgebliebene abschnitte nicht weg und entspricht damit nicht den besprochenen anforderungen
inzwischen hab ich eines gefunden, das ein unified diff erstellt (das ist das format, das das GNU diff tool verwendet). sieht dann so aus:
fixen via AI in #102 implementiert
wie sieht deiner meinung nach der ideale ablauf beim beheben von problemen aus? (vereinfachen kann ich ja immer noch ausgehend von der idealen version, aber ich möchte nicht unnötig eine nicht ideale version, die eigene schwierigkeiten mit sich bringt, implementieren, bevor die ideale version diskutiert wird)
am besten anhand der beiden bestehenden beispiele:
insbesondere ist mir noch unklar, wie der nutzer den betriebsmodus, in dem automatisiert probleme beheben werden, starten können soll, und wie er darin ein bestimmtes gefundenes problem, von dem er möchte, dass es automatisiert gelöst wird, angeben können soll