Verified Commit 70aaae4f authored by Karel Koci's avatar Karel Koci 🤘

Drop restrict option of Script

Restricting accessible uris for script at first look looks like an very
good feature but in reality it cripples scripts ability and adds no
additional security because if we use https and signatures then there
should be no worry about script badness. And if it's then this is just
small garden wall anyone exploiting https and signatures could go
around just by using same https exploit again and again so this really
doesn't make any sense.
parent 49af043c
......@@ -246,11 +246,6 @@ security::
raise the level, such attempt is reported as an error. If not
specified, the level is deduced from the URI. If the URI is remote,
it doesn't go above `remote`, otherwise it doesn't go above `local`.
restrict::
If the level is `restricted`, this is the match for URIs that may be
referenced. If the level is `restricted` and this is not specified,
a match for the protocol and hostname is constructed. It has no
effect with higher security levels.
ignore::
Ignore certain errors. If they happen, don't process such script,
but continue with the rest. This is a lua table with strings, each
......
......@@ -410,7 +410,7 @@ end
local allowed_script_extras = {
["security"] = utils.arr2set({"string"}),
["restrict"] = utils.arr2set({"string"}),
["restrict"] = utils.arr2set({"string"}), -- This is now obsoleted (not used)
["ignore"] = utils.arr2set({"table"})
}
utils.table_merge(allowed_script_extras, allowed_extras_verification)
......@@ -419,7 +419,6 @@ utils.table_merge(allowed_script_extras, allowed_extras_verification)
We want to insert these options into the new context, if they exist.
]]
local script_insert_options = {
restrict = true,
pubkey = true,
ca = true,
crl = true,
......@@ -464,29 +463,6 @@ function script(result, context, filler, script_uri, extra)
if extra.security and not context:level_check(extra.security) then
error(utils.exception("access violation", "Attempt to raise security level from " .. tostring(context.sec_level) .. " to " .. extra.security))
end
--[[
If it was hard to write, it should be hard to read, but I'll add a small hint
about what it does anyway, to spoil the challenge.
So, take the provided restrict option. If it is not there, fall back to guessing
from the current URI of the script. However, take only the protocol and host
part and convert it into a pattern (without anchors, they are added during
the match).
]]
local restrict = extra.restrict or script_uri:match('^[^:]+:/*[^/]+'):gsub('[%^%$%(%)%%%.%[%]%*%+%-%?]', '%%%0') .. "/.*"
--[[
Now check that the new restrict is at least as restrictive as the old one, if there was one to begin with.
Which means that both the new context and the parent context are "Restricted". However, if the parent is
Restricted, the new one has no other option than to be so as well.
We do so by making sure the old pattern is substring of the new one (with the exception of terminating .*).
We explicitly take the prefix and compare instead of using find, because find uses a pattern. We want
to compare tu patterns, not match one against another.
]]
local parent_restrict_trunc = (context.restrict or ''):gsub('%.%*$', '')
if context.sec_level == sandbox.level("Restricted") and restrict:gsub('%.%*$', ''):sub(1, parent_restrict_trunc:len()) ~= parent_restrict_trunc then
error(utils.exception("access violation", "Attempt to lower URL restriction"))
end
-- Insert the data related to validation, so scripts inside can reuse the info
local merge = {}
for name in pairs(script_insert_options) do
......@@ -494,7 +470,6 @@ function script(result, context, filler, script_uri, extra)
merge[name] = utils.clone(extra[name])
end
end
merge.restrict = restrict
local err = sandbox.run_sandboxed(content, script_uri, extra.security, context, merge)
if err and err.tp == 'error' then
if not err.origin then
......
......@@ -238,49 +238,6 @@ function test_script_pass_validation()
assert_equal("context", result.tp, result.msg)
end
-- Test guessing/setting the restrict option
function test_restrict()
local orig_run_sandboxed = sandbox.run_sandboxed
mock_gen('sandbox.run_sandboxed')
local run_i = 0
local function run(uri, options, restrict, level, err)
local result = orig_run_sandboxed([[
Script(']] .. uri .. [[', { security = 'Restricted']] .. options .. [[ })
]], "test_restrict_chunk" .. tostring(run_i), level, nil, {restrict = "http://some%.host/.*"})
run_i = run_i + 1
if err then
assert_equal("error", result.tp)
assert_equal("access violation", result.reason)
else
assert_equal("context", result.tp, result.msg)
assert_equal(1, #mocks_called)
mocks_called[1].p[4] = "context"
assert_table_equal({
{
f = "sandbox.run_sandboxed",
p = {
"",
uri,
"Restricted",
"context",
{
restrict = restrict
}
}
}
}, mocks_called)
end
mocks_called[1] = nil
end
run('http://some.host/index.cgi', '', 'http://some%.host/.*', "Local")
run('http://some.host/index.cgi', ', restrict = ".*"', ".*", "Local")
run('http://some.host/index.cgi', '', 'http://some%.host/.*', "Restricted")
run('http://some.host/index.cgi', ', restrict = "http://some%.host/subdir/.*"', 'http://some%.host/subdir/.*', "Restricted")
run('http://some.host/index.cgi', ', restrict = "http://some%.host/.*"', 'http://some%.host/.*', "Restricted")
run('http://some.host/index.cgi', ', restrict = "http://other%.host/.*"', 'http://some%.host/.*', "Restricted", true)
run('http://some.host/index.cgi', ', restrict = "http://.*"', 'http://some%.host/.*', "Restricted", true)
end
function test_script_err_propagate()
mocks_reset()
local err = sandbox.run_sandboxed([[
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment