MSet::snippet() returns unescaped text when hi_start and hi_end are both empty

[Arpit Jain]

Hi Xapian developers,

I think Xapian::MSet::snippet() in xapian-core breaks its documented
HTML-escaping guarantee on one fast path: when hi_start and hi_end are both
empty and the text already fits inside length, it returns the caller's text
byte for byte with no escaping. I built and ran a proof of concept against
the released 2.0.0 library; the output is below.

Summary
-------
All of the escaping in snippet generation lives in append_escaping_xml(),
which is only reached from SnipPipe::drain(). The branch at the top of
MSet::Internal::snippet() returns before drain() ever runs, so on that path
nothing is escaped, even though the header docs promise unconditionally
that the returned text is safe for HTML. The result is inconsistent in a
way that is easy to miss: the same call escapes correctly as soon as the
text is longer than length, so short documents come back raw and long ones
do not.

Where it is
-----------
xapian-core/queryparser/termgenerator_internal.cc:836-839, at tag v2.0.0
and on git master:

if (hi_start.empty() && hi_end.empty() && text.size() <= length) {
// Too easy!
return string{text};
}

The guarantee it contradicts is at xapian-core/include/xapian/mset.h:404,
in the doc comment for MSet::snippet(): "The returned text is escaped to
make it suitable for use in HTML (though beware that in upstream releases
1.4.5 and earlier this escaping was sometimes incomplete)". There is no
exception there for empty markers.

What happens
------------
An application that indexes untrusted content and renders results with
snippet() using empty hi_start and hi_end, trusting the documented
escaping, emits the attacker's markup into the page for every document
short enough to hit the branch. That is stored XSS in the embedding
application, and the attacker needs nothing beyond the normal
content-submission path.

To be straight about the limits: this is a library contract violation
rather than a hole in Xapian itself, it only bites callers who pass empty
markers and skip their own escaping, and I have not gone hunting for a
specific downstream that does that. Omega is unaffected because it uses
non-empty markers. What makes it worth fixing anyway is that the
documentation tells callers they do not need to escape, and the escaping
CVE-2018-0499 added in 2018 sits below the branch that skips it.

Proof of concept
----------------
Against xapian-core 2.0.0 (Homebrew bottle, arm64 macOS). The driver uses
the public API only: index one document, run a real query through
QueryParser and Enquire, then call snippet() three ways on the same MSet.
Includes and the main() wrapper elided:

Xapian::WritableDatabase db("/tmp/xsnip/db",
Xapian::DB_CREATE_OR_OVERWRITE);
Xapian::Stem stemmer("english");
Xapian::TermGenerator tg;
tg.set_stemmer(stemmer);
Xapian::Document doc;
tg.set_document(doc);
std::string text = "<script>alert(1)</script> hello world";
tg.index_text(text);
doc.set_data(text);
db.add_document(doc);
db.commit();

Xapian::Enquire enq(db);
Xapian::QueryParser qp;
qp.set_stemmer(stemmer);
qp.set_stemming_strategy(Xapian::QueryParser::STEM_SOME);
enq.set_query(qp.parse_query("hello"));
Xapian::MSet mset = enq.get_mset(0, 10);
unsigned flags = Xapian::MSet::SNIPPET_BACKGROUND_MODEL |
Xapian::MSet::SNIPPET_EXHAUSTIVE;

std::cout << "version : " << Xapian::version_string() << "\n";
std::cout << "input : " << text << "\n";
std::cout << "default markers : " << mset.snippet(text, 500, stemmer) <<
"\n";
std::cout << "empty markers : " << mset.snippet(text, 500, stemmer, flags,
"", "") << "\n";
std::cout << "empty, len=20 : " << mset.snippet(text, 20, stemmer, flags,
"", "") << "\n";

Built with g++ -std=c++17 t.cc -o t $(xapian-config --cxxflags --libs),
then run. Observed output:

version : 2.0.0
input : <script>alert(1)</script> hello world
default markers : <script>alert(1)</script> <b>hello</b> world
empty markers : <script>alert(1)</script> hello world
empty, len=20 : ...hello world

Line 3 is the guarded path and escapes as documented. Line 4 is the same
MSet, same text, same flags, empty markers, and the script tag comes back
raw. Line 5 is that same empty-marker call with length cut to 20 so the
text no longer fits, which skips the branch and escapes again.

Suggested fix
-------------
Either drop the fast path so everything goes through SnipPipe::drain(), or
keep it and run append_escaping_xml() over text into the return string
instead of returning string{text}, which preserves the performance win. If
the intent really is that empty markers mean "give me the raw text", I
would say so in the mset.h comment, but I would still argue against leaving
the code as it stands, because the current rule is not "empty markers mean
raw", it is "empty markers mean raw, but only when the text is short
enough", and no caller can reason about that.

Why I don't think it's a duplicate
----------------------------------
The escaping was added by commit c1986aff, "Add missing XML escaping in
MSet::snippet()", whose message is "We were escaping in some cases, but not
all". That is the CVE-2018-0499 fix. It introduced append_escaping_xml(),
applied it at three sites all inside SnipPipe::drain(), and added a test in
tests/api_snippets.cc. It did not touch MSet::Internal::snippet(). The
empty-marker branch returns before drain() is ever constructed, so the 2018
fix could not have covered it, and the run above shows it still returns raw
text on 2.0.0. This is the leftover "some cases, but not all" case, not a
rediscovery of the fixed ones.

Severity and classification (my read, your call)
------------------------------------------------
Medium. CVSS:3.1/AV:N/AC:H/PR:L/UI:R/S:C/C:L/I:L/A:N. CWE-116, with CWE-79
as the downstream effect. AC is High because the impact depends on a caller
configuration the attacker does not control, and scope is Changed because
the consequence lands in the embedding application's browser context rather
than in Xapian.

Affected versions
-----------------
Verified by reading the source at v1.4.6, v1.4.31, v1.5.2 and v2.0.0, and
on git master; the line is at termgenerator_internal.cc:762-765 in v1.4.31.
The proof of concept was executed against the released 2.0.0 library. I
have not pinned down the first release containing the branch, only that it
predates the 2018 escaping fix.

Tooling
-------
I found this by starting from CVE-2018-0499, reading what its fix commit
actually changed, then looking for paths in snippet() that return without
going through drain(). I used AI assistance while investigating, but I
wrote and compiled the proof of concept myself and the output above is what
it printed against xapian-core 2.0.0, so this is empirically verified
rather than inferred from source. If you publish an advisory or request a
CVE for this, my GitHub handle is arpitjain099. Happy to send a patch for
whichever fix shape you prefer.

Thanks,
Arpit
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.xapian.org/pipermail/xapian-devel/attachments/20260811/95f6beec/attachment.htm>