openssh bugs - Apr 2021 - [Bug 3288] Ignoring comments at end of config file lines broke ProxyCommand with #-sign in script

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

            Bug ID: 3288
           Summary: Ignoring comments at end of config file lines broke
                    ProxyCommand with #-sign in script
           Product: Portable OpenSSH
           Version: 8.5p1
          Hardware: amd64
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: tlsalmin at gmail.com

I wondered why my favorite ProxyCommand stopped working after upgrade
to 8.5:

Host ??_*         
  User root                                                             
  SendEnv TERM=xterm              
  CheckHostIP no          
  ControlPath ~/.ssh/cms/%r@%h:%p
  ControlMaster auto      
  ControlPersist 1m 
  ProxyCommand bash -c 'ssh root at 172.16.249.$((1 + ${0%%_*})) nc
${0#[0-9]*_} $1 -q 0' %h %p
  StrictHostKeyChecking accept-new

Checking with verbose the command is clipped:

debug1: Executing proxy command: exec bash -c 'ssh root at 172.16.249.$((1
+ ${0%_*})) nc ${0

Checking again with strace to make sure the command isn't just clipped
by the printer:

execve("/bin/zsh", ["/bin/zsh", "-c", "exec
bash -c 'ssh
root at 172.16.249.$((1 + ${0%_*})) nc ${0"], 0x556526e9b320 /* 47 vars
*/) = 0

The command is clipped at the # sign. after digging I found this commit
to be the culprit:

tree d9cd1cc34e9b0f2b36080069b0bcaa39dd0152e3                           
parent b755264e7d3cdf1de34e18df1af4efaa76a3c015                         
author dtucker at openbsd.org <dtucker at openbsd.org> Mon Nov 30 05:36:39
2020 +0000                                                              
committer Damien Miller <djm at mindrot.org> Fri Dec 4 13:42:38 2020 +1100

upstream: Ignore comments at the end of config lines in ssh_config,     

similar to what we already do for sshd_config.  bz#2320, with & ok djm@ 

OpenBSD-Commit-ID: bdbf9fc5bc72b1a14266f5f61723ed57307a6db4             

diff --git a/readconf.c b/readconf.c                                    
index 09b5e086..d60eeacf 100644                                         
--- a/readconf.c                                                        
+++ b/readconf.c                                                        
@@ -1,4 +1,4 @@                                                         
-/* $OpenBSD: readconf.c,v 1.342 2020/11/15 22:34:58 djm Exp $ */       
+/* $OpenBSD: readconf.c,v 1.343 2020/11/30 05:36:39 dtucker Exp $ */   
 /*                                                                     
  * Author: Tatu Ylonen <ylo at cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo at cs.hut.fi>, Espoo, Finland
@@ -1899,7 +1899,7 @@ read_config_file_depth(const char *filename,
struct passwd *pw,                                                      
     int flags, int *activep, int *want_final_pass, int depth)          
 {                                                                      
        FILE *f;                                                        
-       char *line = NULL;                                              
+       char *cp, *line = NULL;                                         
        size_t linesize = 0;                                            
        int linenum;                                                    
        int bad_options = 0;                                            
@@ -1930,6 +1930,13 @@ read_config_file_depth(const char *filename,
struct passwd *pw,                                                      
        while (getline(&line, &linesize, f) != -1) {                    
                /* Update line number counter. */                       
                linenum++;                                              
+               /*                                                      
+                * Trim out comments and strip whitespace.              
+                * NB - preserve newlines, they are needed to reproduce 
+                * line numbers later for error messages.               
+                */                                                     
+               if ((cp = strchr(line, '#')) != NULL)                   
+                       *cp = '\0';                                     
                if (process_config_line_depth(options, pw, host,
original_host,                                                          
                    line, filename, linenum, activep, flags,
want_final_pass,                                                        
                    depth) != 0)

To fix it one would have to keep tabs on when it is inside a parameter
and only add the null-termination when it is outside of a parameter.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

--- Comment #1 from Tomi Salminen <tlsalmin at gmail.com> ---
Created attachment 3489
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3489&action=edit
Fix proposal.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

--- Comment #2 from Tomi Salminen <tlsalmin at gmail.com> ---
Pull request https://github.com/openssh/openssh-portable/pull/237

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
I'm not sure this fix is correct either - it solves your particular
case, but doesn't deal with # characters in quoted strings.

Maybe we should just revert the original commit until a comprehensive
fix is ready.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tomek.orzechowski at gmail.com

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
*** Bug 3309 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3489|0                           |1
        is obsolete|                            |
             Status|NEW                         |ASSIGNED
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Created attachment 3528
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3528&action=edit
Use a better tokeniser for ssh/sshd_config parsing

I plan to commit this soon - it switches ssh_config and sshd_config
parsing to the argv_split() tokeniser, and gives this tokeniser the
ability to terminate when it encounters an unquoted '#' character.

This should fix this bug, but also improve quote handling in
configuration files generally. Note that the tokeniser is not used for
command-line arguments (e.g. ProxyCommand), so there should be no
behaviour change there other than fixing the '#' truncation regression.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3302


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3302
[Bug 3302] Tracking bug for openssh-8.7
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
this diff, with a few bugfixes has been committed and will be in
openssh-8.7

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3288

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
closing bugs resolved before openssh-8.9

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.