[RFC,2/6] telemetry: fix escaping of invalid json characters
Checks
Commit Message
For string values returned from telemetry, escape any values that cannot
normally appear in a json string. According to the json spec[1], the
characters than need to be handled are control chars (char value < 0x20)
and '"' and '\' characters.
To handle this, we replace the snprintf call with a separate string
copying and encapsulation routine which checks each character as it
copies it to the final array.
[1] https://www.rfc-editor.org/rfc/rfc8259.txt
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
1 file changed, 47 insertions(+), 1 deletion(-)
Comments
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 23 June 2022 18.43
>
> For string values returned from telemetry, escape any values that
> cannot
> normally appear in a json string. According to the json spec[1], the
> characters than need to be handled are control chars (char value <
> 0x20)
> and '"' and '\' characters.
Correct. Other chars are optional to escape.
>
> To handle this, we replace the snprintf call with a separate string
> copying and encapsulation routine which checks each character as it
> copies it to the final array.
>
> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
> 1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/lib/telemetry/telemetry_json.h
> b/lib/telemetry/telemetry_json.h
> index db70690274..13df5d07e3 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char
> *format, ...)
> return 0; /* nothing written or modified */
> }
>
> +static const char control_chars[0x20] = {
> + ['\n'] = 'n',
> + ['\r'] = 'r',
> + ['\t'] = 't',
> +};
> +
> +/**
> + * @internal
> + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> + * except that it does proper escaping as necessary.
> + * Drops any invalid characters we don't support
> + */
> +static inline int
> +__json_format_str(char *buf, const int len, const char *str)
> +{
> + char tmp[len];
> + int tmpidx = 0;
> +
> + tmp[tmpidx++] = '"';
> + while (*str != '\0') {
> + if (*str < (int)RTE_DIM(control_chars)) {
I would prefer the more explicit 0x20, directly copied from the RFC. RTE_DIM(control_chars) hints that it could change.
> + int idx = *str; /* compilers don't like char type as
> index */
> + if (control_chars[idx] != 0) {
> + tmp[tmpidx++] = '\\';
> + tmp[tmpidx++] = control_chars[idx];
> + }
Consider support for other control characters:
+ else {
+ tmp[tmpidx++] = '\\';
+ tmp[tmpidx++] = 'u';
+ tmp[tmpidx++] = '0';
+ tmp[tmpidx++] = '0';
+ tmp[tmpidx++] = hexchar(idx >> 4);
+ tmp[tmpidx++] = hexchar(idx & 0xf);
+ }
Or just drop them, as you mention in the function's description.
> + } else if (*str == '"' || *str == '\\') {
> + tmp[tmpidx++] = '\\';
> + tmp[tmpidx++] = *str;
> + } else
> + tmp[tmpidx++] = *str;
> + /* we always need space for closing quote and null
> character.
> + * Ensuring at least two free characters also means we can
> always take an
> + * escaped character like "\n" without overflowing
> + */
> + if (tmpidx > len - 2)
If supporting the \u00XX encoding, you need to reserve more than 2 characters here and in related code.
> + return 0;
> + str++;
> + }
> + tmp[tmpidx++] = '"';
> + tmp[tmpidx] = '\0';
> +
> + strcpy(buf, tmp);
> + return tmpidx;
> +}
> +
> /* Copies an empty array into the provided buffer. */
> static inline int
> rte_tel_json_empty_array(char *buf, const int len, const int used)
> @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len,
> const int used)
> static inline int
> rte_tel_json_str(char *buf, const int len, const int used, const char
> *str)
> {
> - return used + __json_snprintf(buf + used, len - used, "\"%s\"",
> str);
> + return used + __json_format_str(buf + used, len - used, str);
> }
>
> /* Appends a string into the JSON array in the provided buffer. */
> --
> 2.34.1
>
On Thu, 23 Jun 2022 20:34:07 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> >
> > For string values returned from telemetry, escape any values that
> > cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value <
> > 0x20)
> > and '"' and '\' characters.
>
> Correct. Other chars are optional to escape.
For json_writer (which I wrote for iproute2 and could have been used here).
The switch handles: \t \n \r \f \b \\ " ' as special cases.
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 23 June 2022 20.40
>
> On Thu, 23 Jun 2022 20:34:07 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Thursday, 23 June 2022 18.43
> > >
> > > For string values returned from telemetry, escape any values that
> > > cannot
> > > normally appear in a json string. According to the json spec[1],
> the
> > > characters than need to be handled are control chars (char value <
> > > 0x20)
> > > and '"' and '\' characters.
> >
> > Correct. Other chars are optional to escape.
>
> For json_writer (which I wrote for iproute2 and could have been used
> here).
> The switch handles: \t \n \r \f \b \\ " ' as special cases.
RFC 8259 chapter 7 says:
All Unicode characters may be placed within the
quotation marks, except for the characters that MUST be escaped:
quotation mark, reverse solidus, and the control characters (U+0000
through U+001F).
I have no preference for either, as long as '/' and other non-control characters are not (unnecessarily) escaped.
Using tested and maintained code like json_writer could be beneficial. If you hold the copyright, there should be no license issues.
On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Thursday, 23 June 2022 20.40
> >
> > On Thu, 23 Jun 2022 20:34:07 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Thursday, 23 June 2022 18.43
> > > >
> > > > For string values returned from telemetry, escape any values that
> > > > cannot
> > > > normally appear in a json string. According to the json spec[1],
> > the
> > > > characters than need to be handled are control chars (char value <
> > > > 0x20)
> > > > and '"' and '\' characters.
> > >
> > > Correct. Other chars are optional to escape.
> >
> > For json_writer (which I wrote for iproute2 and could have been used
> > here).
> > The switch handles: \t \n \r \f \b \\ " ' as special cases.
>
> RFC 8259 chapter 7 says:
>
> All Unicode characters may be placed within the
> quotation marks, except for the characters that MUST be escaped:
> quotation mark, reverse solidus, and the control characters (U+0000
> through U+001F).
>
> I have no preference for either, as long as '/' and other non-control characters are not (unnecessarily) escaped.
>
> Using tested and maintained code like json_writer could be beneficial. If you hold the copyright, there should be no license issues.
>
I will take a look at json_writer.
On Thu, Jun 23, 2022 at 08:34:07PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> >
> > For string values returned from telemetry, escape any values that
> > cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value <
> > 0x20)
> > and '"' and '\' characters.
>
> Correct. Other chars are optional to escape.
>
> >
> > To handle this, we replace the snprintf call with a separate string
> > copying and encapsulation routine which checks each character as it
> > copies it to the final array.
> >
> > [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> > lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
> > 1 file changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/telemetry/telemetry_json.h
> > b/lib/telemetry/telemetry_json.h
> > index db70690274..13df5d07e3 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char
> > *format, ...)
> > return 0; /* nothing written or modified */
> > }
> >
> > +static const char control_chars[0x20] = {
> > + ['\n'] = 'n',
> > + ['\r'] = 'r',
> > + ['\t'] = 't',
> > +};
> > +
> > +/**
> > + * @internal
> > + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> > + * except that it does proper escaping as necessary.
> > + * Drops any invalid characters we don't support
> > + */
> > +static inline int
> > +__json_format_str(char *buf, const int len, const char *str)
> > +{
> > + char tmp[len];
> > + int tmpidx = 0;
> > +
> > + tmp[tmpidx++] = '"';
> > + while (*str != '\0') {
> > + if (*str < (int)RTE_DIM(control_chars)) {
>
> I would prefer the more explicit 0x20, directly copied from the RFC. RTE_DIM(control_chars) hints that it could change.
>
Sure. Just trying to avoid magic constants, but in this case it does make
sense. Alternatively, I considered using space char as the sentinel value,
as first non-control-char allowed.
> > + int idx = *str; /* compilers don't like char type as
> > index */
> > + if (control_chars[idx] != 0) {
> > + tmp[tmpidx++] = '\\';
> > + tmp[tmpidx++] = control_chars[idx];
> > + }
>
> Consider support for other control characters:
> + else {
> + tmp[tmpidx++] = '\\';
> + tmp[tmpidx++] = 'u';
> + tmp[tmpidx++] = '0';
> + tmp[tmpidx++] = '0';
> + tmp[tmpidx++] = hexchar(idx >> 4);
> + tmp[tmpidx++] = hexchar(idx & 0xf);
> + }
>
> Or just drop them, as you mention in the function's description.
>
Yeah, I'd appreciate general feedback on that. Adding support is nice, but
just not sure if we really need it or not.
> > + } else if (*str == '"' || *str == '\\') {
> > + tmp[tmpidx++] = '\\';
> > + tmp[tmpidx++] = *str;
> > + } else
> > + tmp[tmpidx++] = *str;
> > + /* we always need space for closing quote and null
> > character.
> > + * Ensuring at least two free characters also means we can
> > always take an
> > + * escaped character like "\n" without overflowing
> > + */
> > + if (tmpidx > len - 2)
>
> If supporting the \u00XX encoding, you need to reserve more than 2 characters here and in related code.
>
Yep. I avoided supporting it for simplicity for now.
> > + return 0;
> > + str++;
> > + }
> > + tmp[tmpidx++] = '"';
> > + tmp[tmpidx] = '\0';
> > +
> > + strcpy(buf, tmp);
> > + return tmpidx;
> > +}
> > +
> > /* Copies an empty array into the provided buffer. */
> > static inline int
> > rte_tel_json_empty_array(char *buf, const int len, const int used)
> > @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len,
> > const int used)
> > static inline int
> > rte_tel_json_str(char *buf, const int len, const int used, const char
> > *str)
> > {
> > - return used + __json_snprintf(buf + used, len - used, "\"%s\"",
> > str);
> > + return used + __json_format_str(buf + used, len - used, str);
> > }
> >
> > /* Appends a string into the JSON array in the provided buffer. */
> > --
> > 2.34.1
> >
>
On Fri, Jun 24, 2022 at 09:00:38AM +0100, Bruce Richardson wrote:
> On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Thursday, 23 June 2022 20.40
> > >
> > > On Thu, 23 Jun 2022 20:34:07 +0200
> > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > >
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Thursday, 23 June 2022 18.43
> > > > >
> > > > > For string values returned from telemetry, escape any values that
> > > > > cannot
> > > > > normally appear in a json string. According to the json spec[1],
> > > the
> > > > > characters than need to be handled are control chars (char value <
> > > > > 0x20)
> > > > > and '"' and '\' characters.
> > > >
> > > > Correct. Other chars are optional to escape.
> > >
> > > For json_writer (which I wrote for iproute2 and could have been used
> > > here).
> > > The switch handles: \t \n \r \f \b \\ " ' as special cases.
> >
> > RFC 8259 chapter 7 says:
> >
> > All Unicode characters may be placed within the
> > quotation marks, except for the characters that MUST be escaped:
> > quotation mark, reverse solidus, and the control characters (U+0000
> > through U+001F).
> >
> > I have no preference for either, as long as '/' and other non-control characters are not (unnecessarily) escaped.
> >
> > Using tested and maintained code like json_writer could be beneficial. If you hold the copyright, there should be no license issues.
> >
>
> I will take a look at json_writer.
Took a quick look at json_writer, and it's certainly an option. The main
gap compared to what we have in our current implementation is that
json_writer is designed around a stream for output rather than an output
buffer. Now while we can use fmemopen to make our buffer act as a stream
for writing, and the write apis should prevent it overflowing, we still hit
the issue of the result of truncation not being valid json. The current
implementation tries to handle truncation more gracefully in that any
fields which don't fit just don't get added.
I'll think about it a bit more, and see if there is a way that it can be
made to work more cleanly.
/Bruce
PS: just changing the output from a string to a stream on the output socket
I don't believe is an option either, as the socket type used for telemetry
is a SOCK_SEQPACKET where message boundaries are preserved, and a single
read will return the entire telemetry reply.
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 24 June 2022 13.17
>
> On Fri, Jun 24, 2022 at 09:00:38AM +0100, Bruce Richardson wrote:
> > On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Thursday, 23 June 2022 20.40
> > > >
> > > > On Thu, 23 Jun 2022 20:34:07 +0200
> > > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > >
> > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > > Sent: Thursday, 23 June 2022 18.43
> > > > > >
> > > > > > For string values returned from telemetry, escape any values
> that
> > > > > > cannot
> > > > > > normally appear in a json string. According to the json
> spec[1],
> > > > the
> > > > > > characters than need to be handled are control chars (char
> value <
> > > > > > 0x20)
> > > > > > and '"' and '\' characters.
> > > > >
> > > > > Correct. Other chars are optional to escape.
> > > >
> > > > For json_writer (which I wrote for iproute2 and could have been
> used
> > > > here).
> > > > The switch handles: \t \n \r \f \b \\ " ' as special cases.
> > >
> > > RFC 8259 chapter 7 says:
> > >
> > > All Unicode characters may be placed within the
> > > quotation marks, except for the characters that MUST be escaped:
> > > quotation mark, reverse solidus, and the control characters
> (U+0000
> > > through U+001F).
> > >
> > > I have no preference for either, as long as '/' and other non-
> control characters are not (unnecessarily) escaped.
> > >
> > > Using tested and maintained code like json_writer could be
> beneficial. If you hold the copyright, there should be no license
> issues.
> > >
> >
> > I will take a look at json_writer.
>
> Took a quick look at json_writer, and it's certainly an option. The
> main
> gap compared to what we have in our current implementation is that
> json_writer is designed around a stream for output rather than an
> output
> buffer. Now while we can use fmemopen to make our buffer act as a
> stream
> for writing, and the write apis should prevent it overflowing, we still
> hit
> the issue of the result of truncation not being valid json. The current
> implementation tries to handle truncation more gracefully in that any
> fields which don't fit just don't get added.
>
> I'll think about it a bit more, and see if there is a way that it can
> be
> made to work more cleanly.
It sounds like json_writer provides a more advanced API, adding a lot of overhead for wrapping it into the Telemetry library. Since we only need a very simple encoder, perhaps copy-paste-modify is more viable. Or just proceed with your RFC code.
Regardless, the API and underlying code probably needs extra scrutiny, so it doesn't become an attack vector into the control plane of a DPDK application.
>
> /Bruce
>
> PS: just changing the output from a string to a stream on the output
> socket
> I don't believe is an option either, as the socket type used for
> telemetry
> is a SOCK_SEQPACKET where message boundaries are preserved, and a
> single
> read will return the entire telemetry reply.
On Fri, 24 Jun 2022 13:29:46 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 24 June 2022 13.17
> >
> > On Fri, Jun 24, 2022 at 09:00:38AM +0100, Bruce Richardson wrote:
> > > On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > > Sent: Thursday, 23 June 2022 20.40
> > > > >
> > > > > On Thu, 23 Jun 2022 20:34:07 +0200
> > > > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > >
> > > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > > > Sent: Thursday, 23 June 2022 18.43
> > > > > > >
> > > > > > > For string values returned from telemetry, escape any values
> > that
> > > > > > > cannot
> > > > > > > normally appear in a json string. According to the json
> > spec[1],
> > > > > the
> > > > > > > characters than need to be handled are control chars (char
> > value <
> > > > > > > 0x20)
> > > > > > > and '"' and '\' characters.
> > > > > >
> > > > > > Correct. Other chars are optional to escape.
> > > > >
> > > > > For json_writer (which I wrote for iproute2 and could have been
> > used
> > > > > here).
> > > > > The switch handles: \t \n \r \f \b \\ " ' as special cases.
> > > >
> > > > RFC 8259 chapter 7 says:
> > > >
> > > > All Unicode characters may be placed within the
> > > > quotation marks, except for the characters that MUST be escaped:
> > > > quotation mark, reverse solidus, and the control characters
> > (U+0000
> > > > through U+001F).
> > > >
> > > > I have no preference for either, as long as '/' and other non-
> > control characters are not (unnecessarily) escaped.
> > > >
> > > > Using tested and maintained code like json_writer could be
> > beneficial. If you hold the copyright, there should be no license
> > issues.
> > > >
> > >
> > > I will take a look at json_writer.
> >
> > Took a quick look at json_writer, and it's certainly an option. The
> > main
> > gap compared to what we have in our current implementation is that
> > json_writer is designed around a stream for output rather than an
> > output
> > buffer. Now while we can use fmemopen to make our buffer act as a
> > stream
> > for writing, and the write apis should prevent it overflowing, we still
> > hit
> > the issue of the result of truncation not being valid json. The current
> > implementation tries to handle truncation more gracefully in that any
> > fields which don't fit just don't get added.
> >
> > I'll think about it a bit more, and see if there is a way that it can
> > be
> > made to work more cleanly.
>
> It sounds like json_writer provides a more advanced API, adding a lot of overhead for wrapping it into the Telemetry library. Since we only need a very simple encoder, perhaps copy-paste-modify is more viable. Or just proceed with your RFC code.
>
> Regardless, the API and underlying code probably needs extra scrutiny, so it doesn't become an attack vector into the control plane of a DPDK application.
I wrote it based on the model used by some Java library.
Other JSON libraries were more concerned with parsing JSON.
@@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char *format, ...)
return 0; /* nothing written or modified */
}
+static const char control_chars[0x20] = {
+ ['\n'] = 'n',
+ ['\r'] = 'r',
+ ['\t'] = 't',
+};
+
+/**
+ * @internal
+ * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
+ * except that it does proper escaping as necessary.
+ * Drops any invalid characters we don't support
+ */
+static inline int
+__json_format_str(char *buf, const int len, const char *str)
+{
+ char tmp[len];
+ int tmpidx = 0;
+
+ tmp[tmpidx++] = '"';
+ while (*str != '\0') {
+ if (*str < (int)RTE_DIM(control_chars)) {
+ int idx = *str; /* compilers don't like char type as index */
+ if (control_chars[idx] != 0) {
+ tmp[tmpidx++] = '\\';
+ tmp[tmpidx++] = control_chars[idx];
+ }
+ } else if (*str == '"' || *str == '\\') {
+ tmp[tmpidx++] = '\\';
+ tmp[tmpidx++] = *str;
+ } else
+ tmp[tmpidx++] = *str;
+ /* we always need space for closing quote and null character.
+ * Ensuring at least two free characters also means we can always take an
+ * escaped character like "\n" without overflowing
+ */
+ if (tmpidx > len - 2)
+ return 0;
+ str++;
+ }
+ tmp[tmpidx++] = '"';
+ tmp[tmpidx] = '\0';
+
+ strcpy(buf, tmp);
+ return tmpidx;
+}
+
/* Copies an empty array into the provided buffer. */
static inline int
rte_tel_json_empty_array(char *buf, const int len, const int used)
@@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
static inline int
rte_tel_json_str(char *buf, const int len, const int used, const char *str)
{
- return used + __json_snprintf(buf + used, len - used, "\"%s\"", str);
+ return used + __json_format_str(buf + used, len - used, str);
}
/* Appends a string into the JSON array in the provided buffer. */