diff --git a/.dockerignore b/.dockerignore index 7b637e9..39df327 100644 --- a/.dockerignore +++ b/.dockerignore @@ -49,3 +49,4 @@ SECURITY.md .idea/ .vscode/ *.exe +.db_password_rescue diff --git a/.gitignore b/.gitignore index f880d40..ec23d26 100644 --- a/.gitignore +++ b/.gitignore @@ -84,3 +84,8 @@ certs/*.pem certs/*.csr evals/cache/ backups/ + +# Written by scripts/rotate_db_password.py when the rotation lands on the +# account but cannot finish. It holds the new password in clear text and is +# meant to be read once and deleted. +.db_password_rescue diff --git a/Sentora/main.py b/Sentora/main.py index 71be50c..4899c3f 100644 --- a/Sentora/main.py +++ b/Sentora/main.py @@ -464,6 +464,22 @@ def _handle_server_epoch(epoch: str) -> None: if _MAC_ADDRESS: OS_INFO = f"{OS_INFO}|MAC={_MAC_ADDRESS}" +def _os_info_for_log() -> str: + """`OS_INFO` without the MAC address. + + The full string is `platform.platform()` with `|HOST=` and `|MAC=` + appended, and the server needs all of it - that is how a reinstalled agent + is recognised as the same machine. The log does not. + + The hostname is unremarkable in a file that lives on that host. The MAC is + the part with a life outside it: a stable hardware identifier that follows + the machine across networks and reinstalls, sitting in a plain-text file + that support bundles and screenshots routinely include. + """ + return OS_INFO.split("|MAC=")[0] + + + TABLES = [ 'critical_files', 'portscan_result', @@ -687,6 +703,13 @@ def _ingest_socket(): if not INGEST_TLS: return raw context = ssl.create_default_context(cafile=INGEST_CA or None) + # Stated rather than inherited. `create_default_context` has set this + # floor since Python 3.10, so on the interpreter this agent is frozen + # against it changes nothing today - and that is exactly why it is + # worth writing down: the guarantee currently depends on which Python + # the build machine had, and the connection it protects is every byte + # of telemetry this endpoint produces. + context.minimum_version = ssl.TLSVersion.TLSv1_2 return context.wrap_socket(raw, server_hostname=SERVER_HOSTNAME or SERVER_IP) except Exception: raw.close() @@ -2546,7 +2569,7 @@ def main(): # Once, here, rather than on every table send. This is the banner that # used to be repeated thousands of times a day in agent.log. - print(f"[*] Host: {OS_INFO}") + print(f"[*] Host: {_os_info_for_log()}") args = _parse_args() @@ -2620,7 +2643,7 @@ def main(): print(f"[*] Automations Mode: {AUTOMATIONS_MODE} " f"({'server' if (AUTOMATIONS_MODE == 'server' or (AUTOMATIONS_MODE == 'auto' and AUTOMATIONS_API_URL)) else 'db'})") print(f"[*] Public IP (auto-detected): {get_public_ip()}") - print(f"[*] OS Info: {OS_INFO}") + print(f"[*] OS Info: {_os_info_for_log()}") print("[*] Starting agent...") time.sleep(2) diff --git a/Sentora/modules/link.py b/Sentora/modules/link.py index 1cabbe6..45a0ffb 100644 --- a/Sentora/modules/link.py +++ b/Sentora/modules/link.py @@ -239,6 +239,14 @@ def _explain_once_if_the_server_speaks_tls(self) -> None: context = _ssl.SSLContext(_ssl.PROTOCOL_TLS_CLIENT) context.check_hostname = False context.verify_mode = _ssl.CERT_NONE + # Not to protect this handshake - nothing is sent over it, and + # verification is off two lines above. It bounds what the + # message below is allowed to claim: a server that only speaks + # TLS 1.0 should not produce "this is serving TLS, switch to + # https://", because switching would be to something worse than + # what the agent has now. The probe answering "no" there is the + # right answer. + context.minimum_version = _ssl.TLSVersion.TLSv1_2 with context.wrap_socket(raw, server_hostname=host): pass except Exception: diff --git a/core/login_guard.py b/core/login_guard.py index 94a8b34..32b75ea 100644 --- a/core/login_guard.py +++ b/core/login_guard.py @@ -43,15 +43,26 @@ def _trusted_networks(): + """Parse TRUSTED_PROXIES, naming the position of anything that fails. + + The position, not the value. `TRUSTED_PROXIES` is a list of proxy + addresses and is not a secret - but this branch runs precisely when + somebody has put something in it that is *not* an address, and the most + common way that happens is a value pasted into the wrong variable. Echoing + it into the container log turns a typo into a disclosure, and the entry + number is what an operator needs to find it anyway. + """ nets = [] - for raw in TRUSTED_PROXIES.split(","): + for position, raw in enumerate(TRUSTED_PROXIES.split(","), start=1): raw = raw.strip() if not raw: continue try: nets.append(ipaddress.ip_network(raw, strict=False)) except ValueError: - print(f"[!] TRUSTED_PROXIES: {raw!r} is not an address or CIDR, ignored", + print(f"[!] TRUSTED_PROXIES: entry {position} is not an address or " + f"CIDR and is ignored ({len(raw)} characters). Nothing " + f"behind it is treated as a trusted proxy.", flush=True) return nets diff --git a/frontend/src/lib/luceneQuery.test.ts b/frontend/src/lib/luceneQuery.test.ts new file mode 100644 index 0000000..3a56dfa --- /dev/null +++ b/frontend/src/lib/luceneQuery.test.ts @@ -0,0 +1,103 @@ +/** + * The builder has to produce a query that means what was clicked. + * + * CodeQL called the old version incomplete sanitisation, and the label is + * misleading here: the query box next to the builder is editable by design, so + * an operator can type raw Lucene whenever they like and there is no boundary + * being crossed. What was actually broken is fidelity — two shapes of value + * produced a query that searched for something else, or for nothing. + * + * Both are in here as their own cases, because they are the ones that were + * wrong rather than the ones that are easy to write. + * + * Run: + * cd frontend + * npm test + */ + +import { test, describe } from 'node:test'; +import assert from 'node:assert/strict'; + +import { quoteValue, toQuery } from './luceneQuery.ts'; + +describe('quoteValue', () => { + test('a plain token is left alone', () => { + // Quoting everything would work, and would make `foo*` stop being a + // wildcard for somebody who typed one into the value box. + assert.equal(quoteValue('CRITICAL'), 'CRITICAL'); + assert.equal(quoteValue('agent-01'), 'agent-01'); + assert.equal(quoteValue('user@example.com'), 'user@example.com'); + assert.equal(quoteValue('4.18.25080.5'), '4.18.25080.5'); + }); + + test('a trailing backslash does not escape the closing quote', () => { + // The bug. `a\` became `"a\"` — the quote escaped, the phrase running on + // into whatever came after it in the query. + assert.equal(quoteValue('a\\'), '"a\\\\"'); + }); + + test('a Windows path survives intact', () => { + // The everyday version of the same thing: every path in this product's + // telemetry has backslashes in it. + assert.equal(quoteValue('C:\\Users\\jdoe'), '"C:\\\\Users\\\\jdoe"'); + }); + + test('backslashes are escaped before quotes, not after', () => { + // Escaping the quote first turns `"` into `\"`, and the backslash pass + // then escapes the backslash that pass just added — producing `\\"`, + // which is an escaped backslash followed by a live quote. + assert.equal(quoteValue('say "hi"'), '"say \\"hi\\""'); + assert.equal(quoteValue('a\\"b'), '"a\\\\\\"b"'); + }); + + test('a value with no whitespace is still quoted when it needs it', () => { + // The old rule only quoted on whitespace or a quote, so this went in raw + // and changed the structure of the query rather than the term. + assert.equal(quoteValue('a)OR(b'), '"a)OR(b"'); + assert.equal(quoteValue('x:y'), '"x:y"'); + assert.equal(quoteValue('a&&b'), '"a&&b"'); + }); + + test('whitespace still forces quoting', () => { + assert.equal(quoteValue('two words'), '"two words"'); + }); +}); + +describe('toQuery', () => { + test('an empty filter set is an empty query', () => { + assert.equal(toQuery([]), ''); + }); + + test('filters with no field or no value are dropped', () => { + assert.equal(toQuery([ + { field: '', op: ':', value: 'x' }, + { field: 'severity', op: ':', value: '' }, + ]), ''); + }); + + test('negation is spelled NOT field:value', () => { + assert.equal( + toQuery([{ field: 'severity', op: '!=', value: 'INFO' }]), + 'NOT severity:INFO'); + }); + + test('several filters join with AND', () => { + assert.equal( + toQuery([ + { field: 'severity', op: ':', value: 'CRITICAL' }, + { field: 'agent', op: '!=', value: 'WIN-01' }, + ]), + 'severity:CRITICAL AND NOT agent:WIN-01'); + }); + + test('a path value produces a query that still parses', () => { + const query = toQuery([ + { field: 'message', op: ':', value: 'C:\\Windows\\Temp\\svc.exe' }, + ]); + assert.equal(query, 'message:"C:\\\\Windows\\\\Temp\\\\svc.exe"'); + // The quotes that remain are the pair that delimits the phrase: every + // other one would have to be escaped, and there are none here. + const unescaped = query.replace(/\\\\/g, '').replace(/\\"/g, ''); + assert.equal((unescaped.match(/"/g) || []).length, 2); + }); +}); diff --git a/frontend/src/lib/luceneQuery.ts b/frontend/src/lib/luceneQuery.ts new file mode 100644 index 0000000..a3239d3 --- /dev/null +++ b/frontend/src/lib/luceneQuery.ts @@ -0,0 +1,53 @@ +/** + * Turning a clicked filter into the query the server actually runs. + * + * The query box on the Search page is editable on purpose — somebody who knows + * Lucene should never be fighting the builder — so this is not a sanitiser and + * there is no privilege to escalate: the operator can type whatever they like + * into the box beside it, and the server runs it either way. + * + * What it has to be is *faithful*. Clicking `message = C:\Users\x` has to + * produce a query that searches for `C:\Users\x`, and the previous version did + * not: + * + * value.replace(/"/g, '\\"') // escapes the quote, not the backslash + * + * so `a\` became `"a\"` — the closing quote escaped, the string running on into + * whatever followed. And the unquoted branch was worse, because it only quoted + * when the value contained whitespace or a quote: `a)OR(b` has neither, went in + * raw, and changed the shape of the query. + * + * The rule here is the other way round. A value that is anything other than a + * plain token gets quoted, and inside the quotes the two characters that mean + * something to Lucene — the backslash and the quote — are escaped, backslash + * first. Escaping the quote first would then escape the backslash it just + * added, which is the classic way to get this exactly wrong. + */ + +export type Filter = { field: string; op: ':' | '!='; value: string }; + +/** Characters a value may contain and still go in bare. + * + * Deliberately a small allow-list rather than a list of things to escape: the + * Lucene metacharacter set is `+ - && || ! ( ) { } [ ] ^ " ~ * ? : \ /` plus + * whitespace, and enumerating it is how one gets missed. Anything outside + * word characters, dot, at-sign and hyphen is quoted. + */ +const PLAIN_TOKEN = /^[\w.@-]+$/; + +export function quoteValue(value: string): string { + if (PLAIN_TOKEN.test(value)) return value; + // Backslash first. The other order escapes the backslashes this line adds. + const escaped = value.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + return `"${escaped}"`; +} + +export function toQuery(filters: Filter[]): string { + return filters + .filter((f) => f.field && f.value) + .map((f) => { + const value = quoteValue(f.value); + return f.op === ':' ? `${f.field}:${value}` : `NOT ${f.field}:${value}`; + }) + .join(' AND '); +} diff --git a/frontend/src/pages/Search.tsx b/frontend/src/pages/Search.tsx index 0a32c25..5f69dc5 100644 --- a/frontend/src/pages/Search.tsx +++ b/frontend/src/pages/Search.tsx @@ -20,6 +20,7 @@ import { useCallback, useEffect, useMemo, useState } from 'react'; import { Link } from 'react-router-dom'; import { Search as SearchIcon, Plus, X, ChevronLeft, ChevronRight } from 'lucide-react'; import api from '../services/api'; +import { toQuery, type Filter } from '../lib/luceneQuery'; import { PageHeader, Card, Badge, DataTable, Row, Cell, EmptyState, ErrorState, LoadingState, Tone, @@ -27,7 +28,6 @@ import { type Tab = 'logs' | 'events'; -type Filter = { field: string; op: ':' | '!='; value: string }; const RANGES = [ { label: '1h', from: 'now-1h' }, @@ -41,17 +41,14 @@ const SEVERITY_TONE: Record = { low: 'low', info: 'info', }; -/** Filters -> the Lucene the server runs. The one place the two agree. */ -function toQuery(filters: Filter[]): string { - return filters - .filter((f) => f.field && f.value) - .map((f) => { - const value = /[\s"]/.test(f.value) ? `"${f.value.replace(/"/g, '\\"')}"` - : f.value; - return f.op === ':' ? `${f.field}:${value}` : `NOT ${f.field}:${value}`; - }) - .join(' AND '); -} +/* `toQuery` now lives in `lib/luceneQuery.ts`, with tests. + * + * It was wrong in two ways that only show up on real values: it escaped the + * quote but not the backslash, so a Windows path ran off the end of its own + * phrase, and it only quoted when the value held whitespace or a quote, so + * `a)OR(b` went in bare and changed the structure of the query rather than + * the term. Both are fidelity bugs — the box beside the builder is editable + * by design, so there was never a boundary here to cross. */ export default function Search() { const [tab, setTab] = useState('logs'); diff --git a/scripts/rotate_db_password.py b/scripts/rotate_db_password.py index 3ed48f5..4ecfbea 100644 --- a/scripts/rotate_db_password.py +++ b/scripts/rotate_db_password.py @@ -17,9 +17,13 @@ 3. reconnect from scratch with the new one (proves the change took) 4. only then write .env -If step 3 or 4 fails after the ALTER has landed, the new password is printed -so the deployment is recoverable by hand. That is the one case where printing -a secret to the terminal beats losing access to the database. +If step 3 or 4 fails after the ALTER has landed, the new password is written to +`.db_password_rescue` beside `.env`, owner-readable only, and the path is +printed. The account has already changed at that point, so a password nobody +has is a database nobody can reach - but the recovery does not need the secret +in terminal scrollback, a CI job log or the screenshot somebody takes of the +failure. Printing it is kept for the one case left: the rescue file itself +cannot be written. Connects to the *published* port, not DB_HOST: `db` only resolves inside the compose network, while the host sees 127.0.0.1:3307. @@ -28,6 +32,7 @@ from __future__ import annotations import argparse +import os import re import secrets import sys @@ -53,6 +58,20 @@ def read_env(path: Path) -> dict[str, str]: return values +def _owner_only(path: Path) -> None: + """Restrict a file to its owner where the platform supports it. + + Best effort. On Windows the POSIX mode is largely cosmetic and the real + control is the ACL, so a failure here must not stop a rotation that has + already altered the account - the same reasoning as `certs/generate_certs` + applies to its private keys. + """ + try: + os.chmod(path, 0o600) + except OSError: + pass + + def write_env_value(path: Path, key: str, value: str) -> None: lines = path.read_text(encoding="utf-8").splitlines(keepends=True) pattern = re.compile(rf"^\s*{re.escape(key)}\s*=") @@ -68,6 +87,12 @@ def write_env_value(path: Path, key: str, value: str) -> None: lines.append(f"{key}={value}\n") path.write_text("".join(lines), encoding="utf-8") + # `.env` holds this password in clear text by design - it is how every + # service in the stack is configured, and there is no version of this + # product where it does not. What is not by design is the file being + # world-readable, which is the half of "clear-text storage" that can + # actually be addressed. + _owner_only(path) def main() -> int: @@ -139,11 +164,36 @@ def main() -> int: except Exception as e: if altered: + # Written to a file, not printed. + # + # The recovery has to exist: the account has already been altered, + # so a password nobody has is a database nobody can reach. What + # does not have to happen is the secret landing in terminal + # scrollback, a CI job log, a `script` capture or the screenshot + # somebody takes of the failure to ask about it. A file the owner + # alone can read gives the same recovery with none of that. + rescue = ENV_PATH.with_name(".db_password_rescue") + try: + rescue.write_text(f"DB_PASSWORD={new_password}\n", encoding="utf-8") + _owner_only(rescue) + where = str(rescue) + except OSError as write_error: + # If it cannot be written, printing beats losing the database - + # which is the trade the original made unconditionally. + print("\n" + "=" * 68) + print("[!] The password WAS changed, the step after it failed, " + f"and the rescue file could not be written ({write_error}).") + print(f" {e}") + print("\n This is the only copy. Save it now:") + print(f" DB_PASSWORD={new_password}") # nosec - see above + print("=" * 68) + return 1 + print("\n" + "=" * 68) print("[!] The password WAS changed, but the step after it failed:") print(f" {e}") - print("\n Save this now — .env may not have been updated:") - print(f" DB_PASSWORD={new_password}") + print(f"\n The new password is in {where} (owner-readable only).") + print(" Put it in .env under DB_PASSWORD, then delete that file.") print("=" * 68) else: print(f"[!] Failed before altering anything, password unchanged: {e}") diff --git a/tests/test_agent_logging.py b/tests/test_agent_logging.py index e9481f1..a84f473 100644 --- a/tests/test_agent_logging.py +++ b/tests/test_agent_logging.py @@ -62,7 +62,31 @@ def test_the_host_banner_is_not_repeated_per_send(): """4376 copies of the same constant string in thirteen hours.""" text = MAIN.read_text(encoding="utf-8") assert 'sent (IP: {public_ip}, OS: {OS_INFO})' not in text - assert '[*] Host: {OS_INFO}' in text, "the banner is no longer logged at all" + # Matched on the banner rather than on the exact interpolation. The MAC + # address was later dropped from what is printed - it is still in + # `OS_INFO` because the server handshake needs it - and pinning the old + # expression made that edit look like the banner had been removed. + assert '[*] Host: ' in text, "the banner is no longer logged at all" + assert text.count('[*] Host: ') == 1, "the banner is logged more than once" + + +def test_the_banner_does_not_carry_the_mac_address(): + """`OS_INFO` is `platform.platform()` plus `|HOST=` and `|MAC=`. + + The server needs all of it — that is how a reinstalled agent is recognised + as the same machine. The log does not. The hostname is unremarkable in a + file that lives on that host; the MAC is a stable hardware identifier that + follows the machine across networks and reinstalls, sitting in a plain-text + file that support bundles and screenshots routinely include. + """ + text = MAIN.read_text(encoding="utf-8") + assert "_os_info_for_log" in text + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("print(") and "OS_INFO" in stripped: + assert "_os_info_for_log" in stripped, ( + f"prints the full OS_INFO, including the MAC: {stripped}" + ) def test_soar_does_not_draw_rules_around_every_cycle(): diff --git a/tests/test_search.py b/tests/test_search.py index 18230fe..8a7fae3 100644 --- a/tests/test_search.py +++ b/tests/test_search.py @@ -231,7 +231,11 @@ def test_the_filters_and_the_text_are_the_same_thing(): """A builder that hides the query teaches nothing, and a bare text box helps nobody on their first day.""" page = PAGE.read_text(encoding="utf-8") - assert "function toQuery" in page + # `toQuery` moved to `lib/luceneQuery.ts` so it could be tested: it was + # escaping the quote but not the backslash, so a Windows path ran off the + # end of its own phrase. What this test is about is unchanged - clicking a + # filter writes the query box, rather than the two being separate things. + assert "from '../lib/luceneQuery'" in page assert "setQuery(toQuery(next))" in page