bugfindutils - Bugs: bug #54236, Leftover extra chars after C1...

 
 

bug #54236: Leftover extra chars after C1 control's tty sanitization

Submitter:  Egmont Koblinger <egmont>
Submitted:  Mon 02 Jul 2018 09:38:37 PM UTC
   
 
Category:  find Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  jay
Originator Name:  Open/Closed:  Closed
Release:  4.6.0 Fixed Release:  4.7.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 05 Jan 2021 02:27:39 PM UTC, comment #7: 

Yes, it seems "workaround" is not the correct word.

This issue was fixed in v4.7.0, thus I'm hereby marking the issue as such.

Bernhard Voelker <berny>
Group administrator
Sat 07 Jul 2018 09:57:11 PM UTC, comment #6: 

Thanks guys!

Probably both patches are okay, but I cannot test them now, so I just trust you that they're indeed correct.

Nit: I don't get why you call it a "workaround" in the commit message. Isn't this the correct fix?

Egmont Koblinger <egmont>
Sat 07 Jul 2018 09:49:45 PM UTC, comment #5: 

Indeed, however I'd rather fix the place which introduced the
problem: in qmark_chars().

Patch attached.



(file #44519)

Bernhard Voelker <berny>
Group administrator
Thu 05 Jul 2018 12:38:16 PM UTC, comment #4: 

I believe that the attached patch fixes the immediate problem.

(file #44508)

James Youngman <jay>
Group administrator
Thu 05 Jul 2018 11:35:36 AM UTC, comment #3: 

Sorry guys, I don't get what you're talking about, and I'm afraid you didn't understand my bugreport.

Various quoting styles (as coreutils's "ls" has them) is only marginally relevant, and I can't see any security implication there, it's primarly about user convenience (as long as you don't emit ESC and similar characters). There's nothing wrong in emitting "somewhat special" (but printable) characters (such as space) unquoted.

My bugreport is about the handling of escape sequences, which control the terminal emulator in various ways. And "find" already handles them specially, since it replaces them by a "?" before printing to the terminal. So the intent is correct. There's nothing to redesign here, at least not as part of this issue.

This one is a mere off-by-one-ish bug, probably a memmove() not including the terminating 0 byte, or something similar, resulting in some leftover garbage at the end of the string. This one is simply an implementation error in replacing the two-byte UTF-8 character by a "?".

Egmont Koblinger <egmont>
Thu 05 Jul 2018 09:42:34 AM UTC, comment #2: 

I agree with the logic of the patch.   I think this problem is important enough though that we should re-order the flow a bit, to make shell_escape_quoting_style) the default, and then override it only when we think that using no quoting is likely safe (e.g. because the output is a plain file or a pipe).

Perhaps we should also pre-select a safe default inside get_new_pred() as defence-in-depth.


James Youngman <jay>
Group administrator
Wed 04 Jul 2018 11:59:29 PM UTC, comment #1: 

To be on the safe side, file names have to be quoted when writing to a terminal.

----------------->8-----------------
diff --git a/find/parser.c b/find/parser.c
index d6621506..6cf4199a 100644
--- a/find/parser.c
+++ b/find/parser.c
@@ -3400,6 +3400,10 @@ open_output_file (const char *path, struct format_val *p)
     }
 
   p->dest_is_tty = stream_is_tty (p->stream);
+
+  /* Fall back to a safer quoting style if writing to a terminal.  */
+  if (p->dest_is_tty)
+    set_quoting_style (p->quote_opts, shell_escape_quoting_style);
 }
 
 static void
-----------------8<------------------

On a terminal, this leads to

  $ find
  .
  './alert'$'\302\233\302\233\302\233\302\233\302\233\302\233\302\233\302\233\033''[31mred'

like newer ls(1) does, and still would be printed literally otherwise:

  $ find | od -tx1z
  0000000 2e 0a 2e 2f 61 6c 65 72 74 c2 9b c2 9b c2 9b c2  >.../alert.......<
  0000020 9b c2 9b c2 9b c2 9b c2 9b 1b 5b 33 31 6d 72 65  >..........[31mre<
  0000040 64 0a                                            >d.<
  0000042

@James: I'm afraid that we'll run into the same discussions as
in coreutils' ls wrt/ the default quoting style on a terminal.
But hey, it's for security which should have precedence.
WDYT?

Have a nice day,
Berny

Bernhard Voelker <berny>
Group administrator
Mon 02 Jul 2018 09:38:37 PM UTC, original submission:  

$ touch $'abcdefghi\xC2\x9Bjklmnopqrstuvwxyz'
$ find
.
./abcdefghi?jklmnopqrstuvwxyzz

Notice the double trailing 'z' at the end of the output.

The two-byte C1 control code (U+009B) in this case is replaced by a '?' if the output is sent to a tty. The rest is apparently correctly shifted by 1 byte in some internal buffer, but I guess the end is not truncated (or the length is not decremented).

This can even have security implications, as the leftover part is not subject to escape sequence sanitization. This way e.g. an ordinary user who creates a specially named file can drive root's terminal emulator to do tricky things. Example where the output actually turns red:

$ touch $'alert\xC2\x9B\xC2\x9B\xC2\x9B\xC2\x9B\xC2\x9B\xC2\x9B\xC2\x9B\xC2\x9B\e[31mred'
$ find
.
./alert?????????[31mredred
                       ^^^
this is actually printed in red in gnome-terminal (and presumably any other terminal emulator that supports C1 control characters in UTF-8). Other escape sequences can do nastier things, e.g. type certain (but not arbitrary) characters as if they were typed by the user.

"find --version" reports 4.7.0-git; dpkg reports 4.6.0+git+20170828-2. Package from Ubuntu 18.04.

Discovered while answering https://askubuntu.com/q/1051367/398785.

Egmont Koblinger <egmont>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #44519:  0001-Shorten-output-of-qmark_chars-when-replacing-a-multi.patch added by berny (851B - text/x-patch - [PATCH] Shorten output of qmark_chars when replacing a multibyte character)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jay (Posted a comment)
  • -email is unavailable- added by berny (Posted a comment)
  • -email is unavailable- added by egmont (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-05 berny Fixed ReleaseNone 4.7.0
    2021-01-05 berny StatusConfirmed Fixed
        Assigned toNone jay
        Open/ClosedOpen Closed
    2018-07-07 berny Attached File- Added 0001-Shorten-output-of-qmark_chars-when-replacing-a-multi.patch, #44519
    2018-07-05 jay Attached File- Added 0001-Shorten-output-of-qmark_chars-after-replacing-a-mult.patch, #44508
    2018-07-04 berny StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code