Stop the one-line install handing a standard user a way in
The bootstrap command staged its script into C:\Windows\Temp and then ran it elevated. That directory grants Users create-file with CREATOR OWNER full control, so somebody without admin rights can pre-plant the exact path the elevated run is about to execute. It now stages under Program Files, creates the directory with an explicit DACL when it is missing, and refuses to run a staged file that is not owned by an admin principal. A failed hardening is loud rather than silent: the run stops instead of carrying on with a file it could not protect. The token moved out of the query string into a header. In a query string it lands verbatim in the reverse proxy and IIS logs, which is a credential written to a file nobody thinks of as a secret store. The query form is still accepted, because bays provisioned before this change are holding install commands that use it, and it logs that it was used - without logging the token - so a site can see who still needs a re-issued command before turning it off with plugin.endpoints.bootstrapquerytoken. Two smaller things this also closes: A request with no token at all answered 400 naming the header, which told an anonymous caller the state of that switch and contradicted the module's own promise that anything unusable answers 404. No token is now 404. The 400 is kept for a token genuinely presented in the query form while the query form is off, which is a caller who needs to know their command is stale. The rollout documentation was republishing the vulnerable one-liner verbatim, so a tech following the runbook reinstated exactly what this fixes. Both documents now carry the current command. Also corrects the operator-facing copy for the token, which named a query argument and a header that do not exist: it is ?t= and X-Endpoint-Bootstrap-Token, per bootstrap.py's own constants. Anybody following the old text got a 404 with no reason given. Both switches added in this range are now declared with the other plugin settings and reachable from their settings page. Read through Setting.get but never declared, they were rows that did not exist until someone wrote one by hand, which on an air-gapped box means the feature was unreachable.
This commit is contained in:
cproudlock
@@ -11,6 +11,7 @@ import datetime
|
||||
import hashlib
|
||||
import io
|
||||
import json
|
||||
import logging
|
||||
import zipfile
|
||||
|
||||
import pytest
|
||||
@@ -27,17 +28,51 @@ TOKENS_URL = '/api/endpoints/siteagent/bootstrap/tokens'
|
||||
SCRIPT_URL = '/api/endpoints/siteagent/bootstrap.ps1'
|
||||
PACKAGE_URL = '/api/endpoints/siteagent/bootstrap/package'
|
||||
BASE_URL = 'https://shopdb.test/shopdb'
|
||||
TEMP_SCRIPT = r'C:\Windows\Temp\shopdb-bootstrap.ps1'
|
||||
TOKEN_HEADER = 'X-Endpoint-Bootstrap-Token'
|
||||
QUERY_SETTING = 'plugin.endpoints.bootstrapquerytoken'
|
||||
# Admin-only staging, NOT C:\Windows\Temp: a standard user can pre-plant a
|
||||
# path there and have it run elevated.
|
||||
STAGE_DIR = r'C:\Program Files\ShopDB\bootstrap'
|
||||
STAGED_SCRIPT = STAGE_DIR + r'\shopdb-bootstrap.ps1'
|
||||
ADMIN_SID = 'S-1-5-32-544'
|
||||
SYSTEM_SID = 'S-1-5-18'
|
||||
# The marker the admin "Download .ps1" link carries, so the server can tell an
|
||||
# admin's browser click apart from a bay still on a pre-change pasted command.
|
||||
UI_LINK_QUERY = 'via=download'
|
||||
|
||||
|
||||
def _expected_command(secret, base=BASE_URL):
|
||||
"""Download, run, delete. Never `irm | iex` - Defender kills that shape."""
|
||||
return ('powershell -ExecutionPolicy Bypass -c "'
|
||||
"try {{ Invoke-WebRequest -UseBasicParsing "
|
||||
"'{0}/api/endpoints/siteagent/bootstrap.ps1?t={1}' "
|
||||
"-OutFile '{2}'; & '{2}' }} finally {{ "
|
||||
"Remove-Item '{2}' -Force -ErrorAction SilentlyContinue }}\""
|
||||
).format(base, secret, TEMP_SCRIPT)
|
||||
"""Stage admin-only, download with the token in a header, check the owner,
|
||||
run, delete. Never `irm | iex` - Defender kills that shape."""
|
||||
return (
|
||||
'powershell -ExecutionPolicy Bypass -c "'
|
||||
"try {{ New-Item -ItemType Directory -Force -Path '{dir}' "
|
||||
'| Out-Null; '
|
||||
"icacls '{dir}' /inheritance:r /grant '*{admin}:(OI)(CI)F' "
|
||||
"'*{system}:(OI)(CI)F' | Out-Null; "
|
||||
'if ((Get-Variable LASTEXITCODE -ValueOnly) -ne 0) '
|
||||
"{{ Write-Host 'Refusing to run: could not restrict "
|
||||
"{dir} to administrators (icacls failed). Fix the directory "
|
||||
"or delete it, then run the command again.' -ForegroundColor Red; "
|
||||
'exit 6 }}; '
|
||||
"Remove-Item '{path}' -Force -ErrorAction SilentlyContinue; "
|
||||
"Invoke-WebRequest -UseBasicParsing "
|
||||
"'{base}/api/endpoints/siteagent/bootstrap.ps1' "
|
||||
"-Headers @{{'{header}'='{secret}'}} -OutFile '{path}'; "
|
||||
"if ((Get-Acl '{path}').GetOwner("
|
||||
'[Security.Principal.SecurityIdentifier]).Value -notin '
|
||||
"@('{admin}','{system}',"
|
||||
'[Security.Principal.WindowsIdentity]::GetCurrent().User.Value)) '
|
||||
"{{ Write-Host 'Refusing to run: the staged bootstrap script is not "
|
||||
"owned by an administrator.' -ForegroundColor Red; exit 5 }}; "
|
||||
"& '{path}' }} finally {{ Remove-Item '{path}' -Force "
|
||||
'-ErrorAction SilentlyContinue }}"'
|
||||
).format(dir=STAGE_DIR, path=STAGED_SCRIPT, base=base, header=TOKEN_HEADER,
|
||||
secret=secret, admin=ADMIN_SID, system=SYSTEM_SID)
|
||||
|
||||
|
||||
def _token_header(secret):
|
||||
return {TOKEN_HEADER: secret}
|
||||
|
||||
|
||||
def _make_cert():
|
||||
@@ -111,8 +146,8 @@ def test_generate_returns_the_secret_and_the_ready_made_command(
|
||||
assert len(secret) == len('sdbb_') + 43
|
||||
assert data['command'] == _expected_command(secret)
|
||||
assert data['scripturl'] == (
|
||||
'{0}/api/endpoints/siteagent/bootstrap.ps1?t={1}'.format(BASE_URL,
|
||||
secret))
|
||||
'{0}/api/endpoints/siteagent/bootstrap.ps1?t={1}&{2}'.format(
|
||||
BASE_URL, secret, UI_LINK_QUERY))
|
||||
token = data['token']
|
||||
assert token['tokenprefix'] == secret[:12]
|
||||
assert token['maxuses'] == 3
|
||||
@@ -136,13 +171,74 @@ def test_the_command_downloads_and_runs_instead_of_piping_into_iex(
|
||||
assert 'iex' not in command.lower()
|
||||
assert '$' not in command
|
||||
assert '\n' not in command
|
||||
assert data['scripturl'] in command
|
||||
assert data['secret'] in command
|
||||
assert "Remove-Item '{0}' -Force".format(TEMP_SCRIPT) in command
|
||||
assert "Remove-Item '{0}' -Force".format(STAGED_SCRIPT) in command
|
||||
assert 'finally' in command
|
||||
# The link the UI offers instead is the route that really serves it.
|
||||
assert client.get(SCRIPT_URL,
|
||||
query_string={'t': data['secret']}).status_code == 200
|
||||
assert client.get(data['scripturl'].split(BASE_URL, 1)[1]
|
||||
).status_code == 200
|
||||
|
||||
|
||||
def test_the_command_stages_where_only_administrators_can_write(
|
||||
client, db, auth_headers):
|
||||
"""C:\\Windows\\Temp lets any standard user create the exact path the
|
||||
command runs elevated, so the staging directory moved under Program Files,
|
||||
is created with an admin-only DACL, and the command refuses to run a staged
|
||||
file owned by anyone but an administrator."""
|
||||
command = _secret(client, auth_headers)['command']
|
||||
|
||||
assert r'C:\Windows\Temp' not in command
|
||||
assert "-OutFile '{0}'".format(STAGED_SCRIPT) in command
|
||||
assert "New-Item -ItemType Directory -Force -Path '{0}'".format(
|
||||
STAGE_DIR) in command
|
||||
assert "icacls '{0}' /inheritance:r".format(STAGE_DIR) in command
|
||||
# Well-known SIDs, so a renamed or localized group cannot widen the grant.
|
||||
assert "/grant '*{0}:(OI)(CI)F' '*{1}:(OI)(CI)F'".format(
|
||||
ADMIN_SID, SYSTEM_SID) in command
|
||||
assert "Get-Acl '{0}'".format(STAGED_SCRIPT) in command
|
||||
assert 'GetOwner([Security.Principal.SecurityIdentifier]).Value' in command
|
||||
assert "-notin @('{0}','{1}',".format(ADMIN_SID, SYSTEM_SID) in command
|
||||
assert '[Security.Principal.WindowsIdentity]::GetCurrent().User.Value' \
|
||||
in command
|
||||
assert 'not owned by an administrator' in command
|
||||
assert 'exit 5' in command
|
||||
# A leftover from an earlier run is cleared before the download, so the
|
||||
# owner test cannot pass on stale content.
|
||||
assert command.index("Remove-Item '{0}'".format(STAGED_SCRIPT)) < \
|
||||
command.index('Invoke-WebRequest')
|
||||
|
||||
|
||||
def test_the_command_sends_the_token_in_a_header_not_the_query_string(
|
||||
client, db, auth_headers):
|
||||
"""A query string lands verbatim in the nginx and gunicorn access logs."""
|
||||
data = _secret(client, auth_headers)
|
||||
command = data['command']
|
||||
|
||||
assert '?t=' not in command
|
||||
assert "-Headers @{{'{0}'='{1}'}}".format(TOKEN_HEADER,
|
||||
data['secret']) in command
|
||||
assert "'{0}/api/endpoints/siteagent/bootstrap.ps1'".format(
|
||||
BASE_URL) in command
|
||||
# The browser download link is the one place that cannot send a header,
|
||||
# and it is marked so its own fetches are not counted as un-migrated bays.
|
||||
assert data['scripturl'].endswith(
|
||||
'?t={0}&{1}'.format(data['secret'], UI_LINK_QUERY))
|
||||
|
||||
|
||||
def test_a_failed_acl_hardening_stops_the_command(client, db, auth_headers):
|
||||
"""icacls is a native command: it raises nothing and $ErrorActionPreference
|
||||
never applies to it, so an unchecked call leaves a staging directory that
|
||||
still inherits the parent DACL and the line downloads into it anyway. The
|
||||
exit code is read sigil-free, because the whole line must hold no `$`."""
|
||||
command = _secret(client, auth_headers)['command']
|
||||
|
||||
assert '(Get-Variable LASTEXITCODE -ValueOnly) -ne 0' in command
|
||||
assert 'LASTEXITCODE' in command and '$LASTEXITCODE' not in command
|
||||
assert 'icacls failed' in command
|
||||
assert 'exit 6' in command
|
||||
# Checked before anything is fetched into the directory, not after.
|
||||
assert command.index('LASTEXITCODE') > command.index('icacls')
|
||||
assert command.index('LASTEXITCODE') < command.index('Invoke-WebRequest')
|
||||
|
||||
|
||||
def test_defaults_are_a_day_and_twenty_five_uses(client, db, auth_headers):
|
||||
@@ -226,8 +322,8 @@ def test_script_is_served_for_a_good_token(client, db, auth_headers):
|
||||
assert response.mimetype == 'text/plain'
|
||||
script = response.get_data(as_text=True)
|
||||
script.encode('ascii')
|
||||
assert '{0}/api/endpoints/siteagent/bootstrap/package?t={1}'.format(
|
||||
BASE_URL, secret) in script
|
||||
assert '"{0}/api/endpoints/siteagent/bootstrap/package"'.format(
|
||||
BASE_URL) in script
|
||||
assert 'Run as administrator' in script
|
||||
assert 'GE-Enforce' in script
|
||||
assert 'SHOPDB_BOOTSTRAP_FORCE' in script
|
||||
@@ -251,6 +347,163 @@ def test_script_bakes_the_staged_binary_hashes(client, db, auth_headers,
|
||||
assert hashlib.sha256(traypayload).hexdigest() in script
|
||||
|
||||
|
||||
def test_the_generated_script_fetches_the_package_with_the_header(
|
||||
client, db, auth_headers):
|
||||
"""Same reason as the one-liner: the package URL must not carry the token,
|
||||
or every agent install writes it into an access log."""
|
||||
secret = _secret(client, auth_headers)['secret']
|
||||
script = client.get(SCRIPT_URL,
|
||||
headers=_token_header(secret)).get_data(as_text=True)
|
||||
|
||||
assert '?t=' not in script
|
||||
assert '$BootstrapPackageHeaders = @{{ "{0}" = "{1}" }}'.format(
|
||||
TOKEN_HEADER, secret) in script
|
||||
assert ('Invoke-WebRequest -Uri $BootstrapPackageUrl '
|
||||
'-Headers $BootstrapPackageHeaders') in script
|
||||
|
||||
|
||||
def test_both_routes_take_the_token_in_a_header(client, db, auth_headers,
|
||||
servercert):
|
||||
secret = _secret(client, auth_headers, maxuses=2)['secret']
|
||||
|
||||
script = client.get(SCRIPT_URL, headers=_token_header(secret))
|
||||
assert script.status_code == 200
|
||||
assert 'ShopDB endpoint agent bootstrap' in script.get_data(as_text=True)
|
||||
assert 'install.ps1' in _files(
|
||||
client.get(PACKAGE_URL, headers=_token_header(secret)))
|
||||
assert EndpointBootstrapToken.query.one().usecount == 1
|
||||
|
||||
|
||||
@pytest.mark.parametrize('url', [SCRIPT_URL, PACKAGE_URL])
|
||||
def test_a_bad_header_token_is_not_found(client, db, auth_headers, url):
|
||||
"""The header is read first, so a good ?t= cannot rescue a bad header - one
|
||||
request carries one token."""
|
||||
secret = _secret(client, auth_headers)['secret']
|
||||
response = client.get(url, query_string={'t': secret},
|
||||
headers=_token_header('sdbb_' + 'a' * 43))
|
||||
assert response.status_code == 404
|
||||
|
||||
|
||||
def test_the_query_form_still_works_and_is_audited_as_deprecated(
|
||||
client, db, auth_headers, caplog):
|
||||
"""A bay running a command pasted before this change must keep working, so
|
||||
the query form stays accepted; it is flagged in the audit row and warned
|
||||
about in the log, with no token value in either."""
|
||||
secret = _secret(client, auth_headers)['secret']
|
||||
with caplog.at_level(logging.WARNING):
|
||||
assert client.get(SCRIPT_URL, query_string={'t': secret},
|
||||
environ_overrides={'REMOTE_ADDR': '10.0.0.6'}
|
||||
).status_code == 200
|
||||
|
||||
row = AuditLog.query.filter_by(entitytype='EndpointBootstrapToken',
|
||||
action='requested').one()
|
||||
assert 'query-string token, deprecated' in row.details
|
||||
assert secret not in row.details
|
||||
warnings = [record.getMessage() for record in caplog.records
|
||||
if record.levelno >= logging.WARNING]
|
||||
assert any('query string' in message and '10.0.0.6' in message
|
||||
for message in warnings)
|
||||
assert all(secret not in message for message in warnings)
|
||||
|
||||
|
||||
def test_the_header_form_is_not_flagged_as_deprecated(client, db, auth_headers,
|
||||
caplog):
|
||||
secret = _secret(client, auth_headers)['secret']
|
||||
with caplog.at_level(logging.WARNING):
|
||||
client.get(SCRIPT_URL, headers=_token_header(secret))
|
||||
|
||||
row = AuditLog.query.filter_by(entitytype='EndpointBootstrapToken',
|
||||
action='requested').one()
|
||||
assert 'deprecated' not in row.details
|
||||
assert not [record for record in caplog.records
|
||||
if record.levelno >= logging.WARNING]
|
||||
|
||||
|
||||
def test_the_admin_download_link_is_not_counted_as_an_un_migrated_bay(
|
||||
client, db, auth_headers, caplog):
|
||||
"""The rollout finds un-migrated bays by the deprecation WARNING, and the
|
||||
UI Download .ps1 link has to use the query form. Marked, it logs at info
|
||||
and is audited as the admin path, so an admin testing a token does not
|
||||
invent a bay that is not there."""
|
||||
data = _secret(client, auth_headers)
|
||||
path = data['scripturl'].split(BASE_URL, 1)[1]
|
||||
with caplog.at_level(logging.INFO):
|
||||
assert client.get(path, environ_overrides={'REMOTE_ADDR': '10.0.0.7'}
|
||||
).status_code == 200
|
||||
|
||||
row = AuditLog.query.filter_by(entitytype='EndpointBootstrapToken',
|
||||
action='requested').one()
|
||||
assert 'admin download link' in row.details
|
||||
assert 'deprecated' not in row.details
|
||||
assert data['secret'] not in row.details
|
||||
assert not [record for record in caplog.records
|
||||
if record.levelno >= logging.WARNING]
|
||||
infos = [record.getMessage() for record in caplog.records
|
||||
if record.levelno == logging.INFO]
|
||||
assert any('Download .ps1' in message and '10.0.0.7' in message
|
||||
for message in infos)
|
||||
assert all(data['secret'] not in message for message in infos)
|
||||
|
||||
|
||||
def test_no_log_or_audit_row_ever_carries_the_token_value(
|
||||
client, db, auth_headers, caplog, servercert):
|
||||
secret = _secret(client, auth_headers, maxuses=4)['secret']
|
||||
with caplog.at_level(logging.DEBUG):
|
||||
client.get(SCRIPT_URL, query_string={'t': secret})
|
||||
client.get(SCRIPT_URL, headers=_token_header(secret))
|
||||
client.get(PACKAGE_URL, query_string={'t': secret})
|
||||
client.get(PACKAGE_URL, headers=_token_header(secret))
|
||||
|
||||
assert all(secret not in record.getMessage()
|
||||
for record in caplog.records)
|
||||
rows = AuditLog.query.filter_by(
|
||||
entitytype='EndpointBootstrapToken').all()
|
||||
assert rows
|
||||
for row in rows:
|
||||
assert secret not in (row.details or '')
|
||||
# The 12-char display prefix is the identifier the UI already shows.
|
||||
assert row.entityname == secret[:12]
|
||||
|
||||
|
||||
@pytest.mark.parametrize('url', [SCRIPT_URL, PACKAGE_URL])
|
||||
def test_the_query_form_can_be_turned_off_once_a_site_has_re_issued(
|
||||
client, db, auth_headers, url):
|
||||
"""Off is the hardened end state, not the default: the UI download link
|
||||
uses the query form too, so a site only turns it off deliberately."""
|
||||
secret = _secret(client, auth_headers)['secret']
|
||||
Setting.set(QUERY_SETTING, False, valuetype='boolean')
|
||||
|
||||
refused = client.get(url, query_string={'t': secret})
|
||||
assert refused.status_code == 400
|
||||
assert TOKEN_HEADER in refused.get_json()['data']['error']['message']
|
||||
# The admin Download .ps1 link is a query token too: the setting is about
|
||||
# a live token landing in an access log, and that link puts one there.
|
||||
assert client.get(url, query_string={'t': secret, 'via': 'download'}
|
||||
).status_code == 400
|
||||
# Refused before any lookup, so it confirms nothing about the token.
|
||||
assert EndpointBootstrapToken.query.one().usecount == 0
|
||||
assert AuditLog.query.filter_by(
|
||||
entitytype='EndpointBootstrapToken', action='requested').count() == 0
|
||||
assert client.get(url, headers=_token_header(secret)).status_code == 200
|
||||
|
||||
|
||||
@pytest.mark.parametrize('url', [SCRIPT_URL, PACKAGE_URL])
|
||||
def test_no_token_is_still_not_found_when_the_query_form_is_off(
|
||||
client, db, auth_headers, url):
|
||||
"""A bare GET presented no token, so it is unusable and answers 404 like
|
||||
every other unusable request. Calling it a query token answered 400 naming
|
||||
the header, which handed an anonymous caller the state of the switch."""
|
||||
_secret(client, auth_headers)
|
||||
Setting.set(QUERY_SETTING, False, valuetype='boolean')
|
||||
|
||||
assert client.get(url).status_code == 404
|
||||
# An empty ?t= presented nothing either.
|
||||
assert client.get(url, query_string={'t': ''}).status_code == 404
|
||||
# And it is the same answer with the query form still on.
|
||||
Setting.set(QUERY_SETTING, True, valuetype='boolean')
|
||||
assert client.get(url).status_code == 404
|
||||
|
||||
|
||||
def test_script_fetch_does_not_spend_a_use(client, db, auth_headers):
|
||||
secret = _secret(client, auth_headers, maxuses=1)['secret']
|
||||
for _ in range(3):
|
||||
@@ -421,3 +674,4 @@ def test_the_command_falls_back_to_the_request_when_no_site_url_is_set(
|
||||
base='https://localhost')
|
||||
assert data['scripturl'].startswith(
|
||||
'https://localhost/api/endpoints/siteagent/bootstrap.ps1?t=')
|
||||
assert data['scripturl'].endswith('&' + UI_LINK_QUERY)
|
||||
|
||||
Reference in New Issue
Block a user