fast_xml requires Expat >=2.4.5 (or backported fixes) for secure operation

[hartwork]

Hi fast_xml,

it has come to my attention that the way fast_xml processes element names at…

fast_xml/c_src/fxml_stream.c

Lines 161 to 191 in 4e8d0ba

	static xmlns_op encode_name(state_t *state, const char *xml_name, ErlNifBinary *buf,
	char **ns_str, char **pfx_str, int top_element)
	{
	const char *parts[3];
	int i, idx = 0;
	for (i = 0; ; i++) {
	if (!xml_name[i] || xml_name[i] == '\n') {
	parts[idx++] = xml_name + i;
	if (!xml_name[i])
	break;
	}
	if (idx >= 3)
	return OP_ERROR;
	}
	const char *ns = NULL, *name = NULL, *prefix = NULL;
	size_t ns_len = 0, name_len = 0, prefix_len = 0;
	if (idx == 1) {
	name = xml_name;
	name_len = parts[0] - xml_name;
	} else {
	ns = xml_name;
	ns_len = parts[0] - xml_name;
	name = parts[0] + 1;
	name_len = parts[1] - parts[0] - 1;
	if (idx == 3) {
	prefix = parts[1] + 1;
	prefix_len = parts[2] - parts[1] - 1;
	}
	}

…requires Expat >=2.4.5 (with the fix to CVE-2022-25236 from pull request(s) libexpat/libexpat#561 (and libexpat/libexpat#577)) to not be vulnerable to injection of namespace separator characters as demonstrated with vulnerable Expat 2.4.4 by the Dockerfile, below.

If everyone uses fast_xml with Expat >=2.4.5, then fast_xml code does not need any changes. I wanted to be sure you're aware.

For the Dockerfile:

# Copyright (c) 2022 Sebastian Pipping <sebastian@pipping.org>
# Licensed under the Apache License version 2.0

FROM debian:sid
SHELL ["/bin/bash", "-c"]
RUN apt-get update \
        && \
    apt-get install --no-install-recommends --yes \
            build-essential \
            ca-certificates \
            cmake \
            git \
            make \
        && \
    git clone --depth 1 --branch R_2_4_4 https://github.com/libexpat/libexpat/ 2>/dev/null \
        && \
    cmake -S libexpat/expat \
        && \
    cp libexpat/expat/examples/elements.c{,_BACKUP} \
        && \
    sed "s/XML_ParserCreate(NULL);/XML_ParserCreateNS(NULL, '|');\n  XML_SetReturnNSTriplet(parser, 1);/" -i libexpat/expat/examples/elements.c \
        && \
    make -j$(nproc) \
        && \
    ! diff -u libexpat/expat/examples/elements.c{_BACKUP,} \
        && \
    ./examples/elements <<<'<a><d xmlns="b&#x7C;c" /><d:c xmlns:d="b" /></a>'

And for the expected output:

# docker build .
[..]
a
        b|c|d
        b|c|d
[..]

Best, Sebastian

[hartwork]

@badlop I saw you merged PR #45. Do you have a second? My main point in this^^ ticket was to raise awareness. Should this ticket be closed as "done, everyone is aware" or is there any party that is not yet aware that may need this ticket? Thanks!

[badlop]

This library is initially used in ejabberd. When ejabberd is installed from Debian or other operating systems, it will get patched expat libraries... but when installed in other ways, that can't be guaranteed. Also, fast_xml is available as a library and apparently other projects are using it: https://hex.pm/packages/fast_xml and who knows what do those projects use, or any other developers...

It would be great if the vulnerability in expat could be prevented in fast_xml. That way users wouldn't need to update to fixed expat versions. Do you have any clue here, or example in other libraries?

I can't help in fixing the source code... I can only help ensuring this is documented and encouraged when the next version is released.

[hartwork]

@badlop thanks for your reply!

It would be great if the vulnerability in expat could be prevented in fast_xml. That way users wouldn't need to update to fixed expat versions. Do you have any clue here, or example in other libraries?

I'm aware of two ways:

• a) make the build system require libexpat >=2.4.5. Downside would be that people who have the fixes but in the form of backports would then need to bypass or patch the check away, say in fast_xml packaging.
• b) add a small C program to precisely test the needed behavior — backported or not —, and make it part of the build system. More work but more precise.

Both of these would rely on users using the original build system.
I have no strong opinions on any of these, these are just options.

[badlop]

Umm, I tried fast_xml with several libexpat versions that I could find in old debian versions (2.2.10, 2.4.7), and all of them give the same result, I guess all them are already fixed.

query:

fxml_stream:parse_element(<<"<a><d xmlns='b&#x7C;c' /><d:c xmlns:d='b' /></a>">>)).

gives this result. Is it patched or vulnerable?

{xmlel,<<"a">>,[],
      [{xmlel,<<"d">>,[{<<"xmlns">>,<<"b|c">>}],[]},
       {xmlel,<<"d:c">>,[{<<"xmlns:d">>,<<"b">>}],[]}]}

[hartwork]

@badlop I was using '|' (pipe, hex character reference |) in my example above but fast_xml is using '\n' (line feed, hex character reference 
) for a namespace separator. If you replace | with 
 in your sample document, you'll get a parse error with Expat >=2.4.5 (or its backported patches) and no error with vulnerable versions.

[badlop]

Aha!

Using libexpat1-dev from debian sid (2.4.7-1) this complains:

fxml_stream:parse_element(<<"<a><d xmlns='b&#xA;c' /><d:c xmlns:d='b' /></a>">>).
{error,{2,<<"syntax error">>}}

But, when using libexpat1-dev from debian stable (2.2.10-2) or from debian testing (2.4.4-1), it gets parsed as:

fxml_stream:parse_element(<<"<a><d xmlns='b&#xA;c' /><d:c xmlns:d='b' /></a>">>).
{xmlel,<<"a">>,[],
       [{xmlel,<<"d:c">>,[{<<"xmlns">>,<<"b\nc">>}],[]},
        {xmlel,<<"d:c">>,[{<<"xmlns:d">>,<<"b">>}],[]}]}

[hartwork]

@badlop yes, but please note that Debian stable — codename bullseye — has Expat 2.2.10-2+deb11u3 with backport fixes in "channel" bullseye-security. So users only have 2.2.10-2 on Debian stable systems that are lacking security updates.

[hartwork]

@badlop PS: With regard to testing/bookworm, I believe testing does not have a -security channel and is not forwarded to 2.4.7-1 of unstable/sid yet, because of a migration blocker that still needs someone to resolve.

[badlop]

Ah, right! I forgot to enable bullseye-security, which includes that backported fix, as you explained.

Thanks! I've added a test to the suite, so packagers and admins can verify it easily.

I'll make sure this is mentioned in the changelog when released.

[hartwork]

Cool, thank you! 👍

[hartwork]

I believe testing does not have a -security channel and is not forwarded to 2.4.7-1 of unstable/sid yet, because of a migration blocker that still needs someone to resolve.

Quick update on that^^: Now resolved, testing/bookworm is at Expat 2.4.7-1 now.