Can not SET variable with same name as POST param

[djyotta]

Introduction

Given the following a POST is made with param content=blah to a sqlpage:

SET $content = sqlpage.variables('post');
SELECT 'text' AS component;
SELECT $content AS contents;

Then the output is like this:

blah

However, if the post param does not have the same name as the variable assigned:

SET $data = sqlpage.variables('post');
SELECT 'text' AS component;
SELECT $data AS contents;

Then the output is like this:

{"content": "blah"}

This is particularly dangerous when using variables internally to run_sql. If any of those variable names are POSTed then the attacker has full control over which sql is run.

SET inner = 'something/safe.sql';
-- meaningless if POST has inner=evil.sql
SELECT 'dynamic' AS content, sqlpage.run_sql($inner) AS properties;

Even this is not safe as the sql is run regardless of the where clause:

SET inner = 'something/safe.sql';
SELECT 'dynamic' AS content, sqlpage.run_sql($inner) AS properties
-- output only included on page if WHERE matches, but sql is still run....
WHERE $inner IN ('whitelisted/path.sql', 'another/safe.sql');

Aside from the security issue, it causes a headache when variable assignment is seemingly of none effect.

Note, this doesn't seem to affect GET , only POST

Version information

• OS: MXLinux
• Database: N/A
• SQLPage Version : 0.20.5, 0.21.0

Additional context

Add any other context about the problem here.

[lovasoa]

The documentation is currently not super clear:

$ParameterName works the same as :ParameterName, but it can also be set through a query parameter in the URL. If you add ?x=1&y=2 to the end of the URL of your page, $x will be set to the string '1' and $y will be set to the string '2'. If a query parameter was not provided, it is set to NULL.

The behavior is:

• :x : post variable set by form
• ?x (where supported) : get variable set through url parameter
• $x : post variable, and when the post variable is not set, get variable.

This has always been the case, and changing it would break applications. But I agree it's confusing, improperly documented, and could lead to security vulnerabilities if used improperly.

The solution for you in the short term is to use SET :x instead of SET $x.

But in the long term, I'm ambivalent about what should be done. What do you think?

[djyotta]

Ah so, would I be correct in thinking the "safest" thing to do for now is use SET :x for internal variables that should never be set by users? ie,

SET :x = 'safe/value.sql';
SELECT 'dynamic' AS component, sqlpage.run_sql(:x) AS properties; -- safe because of prior SET

[djyotta]

A related observation: though I see I've used SET $content; in my example, I personally have been using SET content; (no $) everywhere, including when I hit this issue. It seems SET content; behaves like SET $content; is that correct?

[djyotta]

:x : post variable set by form
?x (where supported) : get variable set through url parameter
$x : post variable, and when the post variable is not set, get variable.

Ah OK, from the docs, my takeaway was to use $x everywhere unless I wanted to specifically discriminate between GET and POST params. But if SET is setting only the GET param (is it?) when using SET $x, then the behaviour I'm seeing makes sense.

Here are my thoughts on current behaviour and potential improvements:

1. It is not clear to me is what SET $x should do or if it is well defined at all
2. Ability to use SET whilst preserving GET/POST params would be helpful
3. A safe way to reference internally set variables that do not fallback to POST or GET params is needed

Proposal:

Symbol	SET	SELECT
&	Write internal var only	Resolve to internal var (if not null) else fallback to POST, then GET. Warn on using with run_sql
!	Write internal var only	Resolve to internal var only. Safe for use with run_sql
$	Existing behaviour	Warn on usage with run_sql and SET. Recommend against using in SELECT - use & instead

---

1. It is not clear to me is what SET $x should do or if it is well defined at all

For example, when referencing the value "reading":

SELECT $x; -- like SELECT COALESCE(:x, ?x)

But during assignment "writing":

SET $x;  -- sets the GET parameter only

For :x and ?x I think the behaviour of the "write" op is obvious given the definition of the "read" op. But for $x it's not intuitive.

I can see how it could make sense the way it is, but on the other hand, it's weird that:

-- :x is 'data'
SET $x = 'blah';
SELECT $x; -- returns 'data'

---

2. Ability to use SET whilst preserving GET/POST params

SET &x = 'blah'; -- doesn't affect GET or POST param, just sets internal var
SELECT &x; -- like SELECT COALESCE(!x, $x); -- favour internal var, then fallback to POST, then GET

I would even argue using the proposed &x instead of $x everywhere would result in expected behaviour for most users.

It would also make comparison of computed var &x with user supplied param $x reveal if a param has been modified internally or not without introducing a placeholder variable with a distinct name to keep the original and modified values separate.

---

3. A safe way to reference internally set variables that do not fallback to POST or GET params

SET !x = 'blah';
SELECT !x; -- NULL if `x` has not been set internally, regardless of presence in GET or POST params

Would make use of !x in run_sql completely safe:

SET !x = 'safe/value.sql';
SELECT 'dynamic' AS component, sqlpage.run_sql(!x) AS properties; -- safe because !x is NULL if not set internally

We could also warn on using sqlpage.run_sql with any of $x, :x, ?x, &x ...: "Potential use of user supplied value in run_sql"

It would also make comparison of computed var !x with user supplied param $x reveal if a param has been modified internally or not, or was even supplied by the user at all without introducing a placeholder variable with a distinct name to keep the original and computed values separate.

---

Introducing the new syntax (or equivalent) would obviously avoid breaking existing code.

Use of SET $x could be discouraged by making it a warning: "Better to use SET &x; or SET !x; ?

Syntax proposed by 3) is most important for security. Syntax proposed by 2) would be a more convenient way of writing COALECE(!x, $x) in a SELECT and could also be the recommended way to reference parameters in general case as it behaves similar to how $x does already.

[lovasoa]

Hello and thank you for your thoughts! We cannot really use new and sqlpage-specific variable syntax, because we wouldn't be able to parse them with a standard sql parser, and because they already have their own semantics (!x means NOT x in MySQL, for instance).

I have another, slightly less ambitious but more realistic proposition:

• immediately update the semantics of SET $x to match the one of SELECT $x: set the post variable if it exists, otherwise set the GET variable. This removes the surprising and error-prone behavior where an user can override a variable by setting a POST parameter.
• add a warning in the logs when using $x to reference a POST parameter. Warn the users that this post OR get behavior is deprecated.
• in a future release, remove the ability to reference POST variables with $x, and clearly document the new, simple, behavior: :x is a POST variable, $x is a GET variable.

[djyotta]

Ah, I didn't realize $ and : where not already sqlpage specific. I mean I knew : is used for placeholders in prepared statements. I guess you have a thinner wrapper around pure SQL than I realized.

I see your point about !. Same would go for & too bitwise and or something.

I guess if we don't introduce any sqlpage specific we're stuck with overloading existing operators that won't mess up the parser.

I like your proposal, it would a big improvement. I think it still lacks ability to create variables that can only be set internally. Would you have any ideas for that could be done? I think it's not required if the dev just sets variables before using them. It would be nice if there was no room for error though...

[lovasoa]

Do you see a concrete case when reusing the same namespace as get variables would be problematic? I mean, the user being able to overwrite the programmer's variables like it is today is terrible, as you have highlighted already. But when this is fixed, I don't feel like we really need more.

[djyotta]

I don't feel very strongly about either of these cases, but since you ask:

Case 1 (security impact) - unset var

-- oops, I never set :inner, now user can dictate which sql is run or worse if sqlpage.exec
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;

Case 1 can be hit without being detected if the var is unset only in rare cases, or if the error coming back from sqlpage.run_sql is squashed (not sure if that is possible).

Being unset (and so NULL) would potentially be invisible but could be exploited.

Case 2 (convenience) - value swap

Consider some SQL (or function if db supports it) normalize.

-- pollute the namespace with :x and :changed
SET ":x" = normalize(:var);
SET ":changed" = :x <> :var;
-- assign the normalized value back
SET ":var" = :x;
-- redirect only if URL changed after normalization
SET ":inner" =  CASE :changed
WHEN TRUE THEN 'redirect.sql'
ELSE 'continue.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;

It would be much nicer (in my opinion)

-- using my notation proposed above for consistency
SET "!var" = normalize(:var);
SET ":inner" = CASE "!var" <> :var
WHEN TRUE THEN 'redirect.sql'
ELSE 'continue.sql'
END;
-- no need to assign back if references to $var later are like COALESCE(!var, :var, ?var)
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;

NOTE: while typing this, I came up with a solution to the value swap using JSON, which is elegant enough for me. I'll probably use that moving forward unless there is a more elegant way to do it that you recommend.

---

My conclusions (rambling for my own benefit - typing it out helps me think about if it makes any sense at all)...

Case 1 - unset var: not a huge risk.

Case 2 - value swap: it's a pain in all languages, but somehow seems particularly clunky in SQL...

If we had multiple return values I could do something like:

-- pollute the namespace with :changed
SET ":var", ":changed" =  normalize(:var); -- postgresql pgSQL ext has SELECT INTO var1, var2, var3 ...
SET ":inner" = CASE ":changed"
WHEN TRUE THEN 'redirect.sql'
ELSE 'continue.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;

Or I guess we can simulate multiple return values via JSON:

-- pollute the namespace with :ret
SET ":ret" =  normalize(:var);
SET ":var" = :ret->>'var';
SET ":inner" = CASE ":ret"->>'changed';
WHEN 'true' THEN 'redirect.sql'
ELSE 'continue.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;

Even if we could declare methods SQLPage (client) side...

DECLARE normalize_and_maybe_redir(var) AS $$
-- local temp var so less namespace pollution (I guess inner scope sees :x)
SET ":x" = normalize(var);
-- assign the normalized value back (only for benefit of inner scope)
SET ":var" = :x;
-- redirect only if URL changed after normalization
SET ":inner" = CASE "!var" <> :var
WHEN TRUE THEN 'redirect.sql'
ELSE THEN 'empty.sql'
END;
SELECT 'dynamic' AS component, sqlpage.run_sql(:inner) AS properties;
-- ideally could pass in :x instead of :var as an argument to run_sql
-- SELECT 'dynamic' AS component, sqlpage.run_sql(:inner, var=:x) AS properties;
RETURN :x;
$$ LANGUAGE sqlpage;

:var = normalize_and_maybe_redir(var);
:var2 = normalize_and_maybe_redir(var2);

... it still looks so clunky. I'm honestly not sure why. Am I missing something?

The JSON solution to the value swap is probably a decent solution and reads better than most of them, and doesn't require any changes.

[lovasoa]

@djyotta , can you test v0.22 ?

[djyotta]

Yes, I will. I've been sick. Still recovering. Will do a test as soon as possible.

[lovasoa]

I wish you a prompt recovery!

[djyotta]

Thanks @lovasoa . I'm feeling a bunch better already. But it must have been something nasty because it took 4 days before I felt like I could function normally again...

I did some tests. I used the following SQL to create a test form to GET and POST.

SET ":post" = sqlpage.variables('post');
SET ":get" = sqlpage.variables('get');

-- set GET variable
SET $x = 'get';
-- set POST variable
SET ":y" = 'post';
-- set POST variable

SELECT 'text' AS component, CASE COALESCE($x, '')
WHEN 'get-blah' THEN 'Fail - $x is overriden by GET param'
WHEN 'post-blah' THEN 'Fail - $x is overriden by POST param'
WHEN 'get' THEN 'Success - assigning $x worked'
WHEN '' THEN 'Nothing assigned to $x'
ELSE 'Fail - assigning $x failed for an unknown reason'
END AS contents;

SELECT 'text' AS component, CASE COALESCE(:y, '')
WHEN 'get-blah' THEN 'Fail - :y is overriden by GET param'
WHEN 'post-blah' THEN 'Fail - :y is overriden by POST param'
WHEN 'post' THEN 'Success - assigning :y worked'
WHEN '' THEN 'Nothing assigned to :y'
ELSE 'Fail - assigning :y failed for an unknown reason'
END AS contents;

SELECT 'debug' AS component;
SELECT $get AS "GET Params";
SELECT :post AS "POST Params";
SELECT 'table' AS component;
SELECT * FROM json_each(sqlpage.variables()) ORDER BY fullkey;


DROP TABLE IF EXISTS test_cases;
CREATE TEMPORARY TABLE test_cases(
	id,
	type text,
	var text
);
INSERT INTO test_cases VALUES (1, 'get', 'x'), (2, 'get', 'y');
INSERT INTO test_cases VALUES (1, 'post', 'x'), (2, 'post', 'y');

SELECT 'button' AS component;
SELECT '/test.sql' AS link, 'Clear' AS title;

SELECT 'form' AS component, 'get' AS method;
SELECT 'submit' AS type, var AS name, 'Test GET Case '||id AS label, 'GET '||var||'=get-blah' AS value FROM test_cases WHERE type = 'get';

SELECT 'form' AS component, 'post' AS method;
SELECT 'submit' AS type, var AS name, 'Test POST Case '||id AS label, 'POST '||var||'=post-blah' AS value FROM test_cases WHERE type = 'post';

I observed the bug is present on v0.20.4 and fixed in 0.22.0

• Specifically Test POST Case 1 fails on 0.20.4. The other cases are for completeness and are passing on both versions.

---

I do wonder about the warning messages though.

In Test POST Case 1, I get Deprecation warning! $x was used to reference a form field value (a POST variable) instead of a URL parameter. This will stop working soon. Please use :x instead.
But that's because the POST has x=post-blah and I'm using SET $x = 'get' and referencing with $x later as the intention is x is a GET param not a POST param. This means that a user sending parameters via POST instead of GET will cause the warning to log. The workaround would be to disallow POST on a GET endpoint which can be done with sqlpage.request_method. Alternatively, directly referencing the GET param with ?x instead of $x would prevent the warning log from appearing too... but neither SET ?x nor SELECT ?x are working for me with SQLite in-memory db.

Otherwise testing seems ok. I've already put some of my production apps on v0.22.0 and they are working fine (but just lots of warnings as I haven't yet fixed my apps to reference :var everywhere instead of $var

You would know better than me if it's behaving as you intended