We're looking into how to best handle this and some related output formatting issues, although the fix more invasive than we'd want to put into 15.08. I might have a relatively simple patch you could use in the meantime if you don't want to wait for a more extensive fix in 16.05.

If your immediate concern is just sacct, I'd suggest looking at --delimiter option. Admittedly your users may pick almost anything which would still cause problems, but something less common like ! may be easier in the meantime. It also allows for multiple characters, #### is just as valid as |. At present it would also permit UTF8 wide-characters like ☃ (Unicode Snowman, U+2603, it may or may not render in your email client) to be used.

Changing the delimiter doesn't address errant newlines, although I'm not sure how common that is for your system at present?

- Tim

OK, thanks.  I'd be interested in the patch.  In the meantime we are looking into using multiple characters.

For the new lines, just escaping them would be fine for our purposes.  We see newlines a couple times a month.  From looking at the jobs and logs it seems like a user may do something like (on the command line):

sbatch --jobname="foo
>bar" --other-slurm-flags....

That is, hit an inadvertent newline while in a quoted string, get the shell continuation prompt, close the quote, and keep typing the command.

I'll see what I can get you in the next day or so. The patch may differ from what we do in 16.05, but would work in the meantime.

The strategy I'm looking at taking is to (a) swap any '\n' for ' ' in strings, and (b) swap the delimiter string (if found, and as many times as found) for the same number of spaces. That also avoids some overhead with C string manipulation by keeping the same array throughout rather than cutting and splicing it back together.

We could look at trying to escape values with \ or similar, but that gets messy fast and I assume most people are parsing the output with `cut` or `awk` or similar which wouldn't necessarily respect the escaped characters anyways. It looks like XDMoD at least probably wouldn't parse the escaped values any better than unescaped? (I'm guessing from the uncommon --parsable2 flag mentioned that may be what you're feeding this into? That, and the fact that your group develops it from what I recall.)

If you're sitting near those folks (or are those folks?) there's an internal feature request to add a JSON output format to some commands which may be of interest to them. It might show up in 16.05, although it's a low priority as no one is sponsoring that work at present.

For 16.05 the strategy may shift to rejecting jobs with bad characters in the various fields (job name is but one of several that are affected - comment, stdout/stderr and a few other fields could also have junk in them) rather than trying to clean up the output later on.

Yeah one of the uses for this is for XDMoD. The other is just some general auditing of jobs. Either escaping or replacing is fine. The output is parsed by either php or Python depending on the code path so we could handle either change.

JSON would be perfect, but understandable that it's low priority for you.

> On Dec 17, 2015, at 4:12 PM, bugs@schedmd.com wrote:
> 
> Tim Wickberg changed bug 2265 
> What	Removed	Added
> CC	da@schedmd.com, jette@schedmd.com	 
> Comment # 4 on bug 2265 from Tim Wickberg
> I'll see what I can get you in the next day or so. The patch may differ from
> what we do in 16.05, but would work in the meantime.
> 
> The strategy I'm looking at taking is to (a) swap any '\n' for ' ' in strings,
> and (b) swap the delimiter string (if found, and as many times as found) for
> the same number of spaces. That also avoids some overhead with C string
> manipulation by keeping the same array throughout rather than cutting and
> splicing it back together.
> 
> We could look at trying to escape values with \ or similar, but that gets messy
> fast and I assume most people are parsing the output with `cut` or `awk` or
> similar which wouldn't necessarily respect the escaped characters anyways. It
> looks like XDMoD at least probably wouldn't parse the escaped values any better
> than unescaped? (I'm guessing from the uncommon --parsable2 flag mentioned that
> may be what you're feeding this into? That, and the fact that your group
> develops it from what I recall.)
> 
> If you're sitting near those folks (or are those folks?) there's an internal
> feature request to add a JSON output format to some commands which may be of
> interest to them. It might show up in 16.05, although it's a low priority as no
> one is sponsoring that work at present.
> 
> For 16.05 the strategy may shift to rejecting jobs with bad characters in the
> various fields (job name is but one of several that are affected - comment,
> stdout/stderr and a few other fields could also have junk in them) rather than
> trying to clean up the output later on.
> You are receiving this mail because:
> You reported the bug.

Created attachment 2530 [details]
strip newlines for user-provided strings, remove delimiter as well

Attached changes the output for sacct/sacctmgr/sreport/sshare/sstat to replace newlines with spaces and remove delimiter character(s) entirely in user-provided strings.

Note this is against 15.08, and does not look like it'll apply back to 14.11 without some adjustment.

I'm still looking into how best to approach this in 16.05, we'll likely add some additional checks up-front and potentially a new flag to enable this, but may not commit anything that changes the output. (This also helps keep the other commands - squeue in particular - free from bad output, as it uses a different set of functions for formatting.)

OK, thanks.  We are going to upgrade to 15.08 in a few weeks so I'll not try to backport this to 14.11.

As you look at solutions for 16.05, I'll note that we've also seen what seems like arbitrary binary (non utf) data in the job name field.  Not sure where that comes from, maybe an errant copy/paste by the user.  This is easier for us to deal with because we know any data like that is bad and we just dump it during processing.

Thanks

On 12/22/2015 01:46 PM, bugs@schedmd.com wrote:
> http://bugs.schedmd.com/show_bug.cgi?id=2265
>
> --- Comment #7 from Martins Innus <minnus@buffalo.edu> ---
> OK, thanks.  We are going to upgrade to 15.08 in a few weeks so I'll not try to
> backport this to 14.11.

Okay, good to hear.

If it's alright with you I'm going to update this bug to reclassify it 
as a Sev5 enhancement for string sanitization in user submissions, with 
a target release for 16.05.

> As you look at solutions for 16.05, I'll note that we've also seen what seems
> like arbitrary binary (non utf) data in the job name field.  Not sure where
> that comes from, maybe an errant copy/paste by the user.  This is easier for us
> to deal with because we know any data like that is bad and we just dump it
> during processing.

That'll come in as part of the input string sanitization. At present any 
user-provided strings are copied into char[]'s and printed out as-is. 
The sanitization step will reject jobs with any of the ASCII control 
characters (ASCII 0-31,127 , which aren't used in UTF8) to avoid this 
and force users to fix their mangled submissions appropriately.

Deciding what are legitimate UTF8 strings isn't something we want to 
tackle. You may still get some oddness, especially if people throw the 
text-direction characters in, but I'm guessing that's less likely.

Marking as Sev5 / enhancement to provide for further sanitization of user-provided strings.

*** Ticket 3375 has been marked as a duplicate of this ticket. ***

*** Ticket 4086 has been marked as a duplicate of this ticket. ***

(In reply to Tim Wickberg from comment #11)
> *** Bug 4086 has been marked as a duplicate of this bug. ***

I would like to ask what the current status of the input sanitization is. It's been a long time that this bug has been open, and, as I pointed out in the bug report that I opened myself, it is a significant security risk. It's next to impossible for job-epilog script writers to harden their scripts against the broken input they get from `scontrol`.

I have taken a look at the current code, but could not find any sanitization - did I overlook something?

*** Ticket 7218 has been marked as a duplicate of this ticket. ***

*** Ticket 7218 has been marked as a duplicate of this ticket. ***

Hi,

  we have this problem as well. The jobname like st:F6|J  cause a movement of the 
 "ResvCPU" and "ResvCPURAW" slurm fields. The output of for example "sacct --allocations --parsable2 --units=M  --format=All -j 1698989" shows  in the header 105 fields and in the output 106 fields. When could we receive a solution of this problem.

Kind regards
Brigitte May

Karlsruher Institut für Technologie (KIT) 
Steinbuch Centre for Computing (SCC)

Brigitte May
Scientific Computing und Simulation (SCS)

Zirkel 2, Gebäude 20.21, Raum 207
76131 Karlsruhe
Telefon: +49 721 608 46422
Fax: +49 721 32550
E-Mail: brigitte.may@kit.edu
Web: http://www.scc.kit.edu

KIT – Die Forschungsuniversität in der Helmholtz-Gemeinschaft

Another place where this makes an issue is where the "Constraints" field may have the "|" character also (e.g. "mem16G|mem32G" ). My processing tool also choked today due to this issue. This output was produced using SLURM 20.02.

Wirawan

I consider this issue easy to fix if we're willing to be a little dirty.
Why don't we do the following to avoid this malady:
We will replace "bad" characters with its C-like escape, e.g. \xNN where
NN is the hexadecimal ASCII code of the character?
In this case, we will replace:

    "\"     =>  \x5c
    "|"     =>  \x7c
    newline =>  \x0a

Ugly? Yes.
Works? Yes.
The approach above is amenable to machine parsing and preserves the data.
After all, those chars should not have been appearing in SLURM job names
if one is reasonable.
Other "bad" characters with ASCII codes lower than 32 can also
be similarly encoded.
(Optionally, if wanted, those higher than 127 can be encoded
in the same way.)
I propose that the fix is done in the `common/print_fields.c` in the
following functions: `print_fields_str` and `print_fields_char_list`.
I believe these are the only vulnerable points.
We make a function that scans the input string (before assigning the
value to `print_this` pointer) for the "forbidden" characters, and if so,
we will make a copy of the string by substituting the forbidden chars
with that long escape.

Optionally, if wanted, those higher than 127 can be encoded in the same way too.
I propose that the fix is done in the `common/print_fields.c` in the
following functions: `print_fields_str` and `print_fields_char_list`.
I believe these are the only vulnerable points.
We make a function that scans the input string (before assigning the
value to `print_this` pointer) for the "forbidden" characters, and if so,
we will make a copy of the string by substituting the forbidden chars
with that long escape.

Please comment whether this is a reasonable approach, before it is implemented.

(In reply to Wirawan Purwanto from comment #17)
> I consider this issue easy to fix if we're willing to be a little dirty.

To head off the rest of the discussion here: I'm not willing to "be a little dirty", and won't accept any patches based on what you've described.

This is still quite an issue for our centre as we keep finding new and unique ways that our friendly researchers can add characters that prevent reporting being carried out. 

I'd like to suggest urlencode (a python function) for this as it will encode all characters into a sane format that could be parsed. Nice industry standard behind it as well.

Thoughts?

Just giving a +1 here, this is affecting our metrics as well. 

Sanitizing the delimiter by replacing it with spaces sounds fine to me, is there any reason why this hasn't made it to a release yet?


I am out of the office until 10/27. If you have questions that require a reply before then please email research@unc.edu.
 Regards,
Virginia Williams
Systems Administrator
Research Computing
UNC Chapel Hill

Created attachment 16862 [details]
Add --escape-delimiter option to sacct


Hi,

I only bothered to search for this bug after I wrote a patch... So here's my patch if anyone wants. It's based on the current master - 2020-11-30, but we currently have 19.05 running here so it's tested only on 19.05.

Default sacct behavior is preserved, and only changes when adding the --escape-delimiter flag. In which case it will add '\' before any other '\' and before the delimiter (a single '\' is added even if the delimiter is more than one character). This patch doesn't take care of newlines or any other special character.

I'm not sure if this is still relevant, as an update to the parsing scripts will also be required in order to take the escapes into account. And I think the future is probably going for the rest api for these sort of things (not relevant for me yet, as we're still on 19.05).

*** Ticket 10853 has been marked as a duplicate of this ticket. ***