Opened 4 years ago

Closed 13 months ago

#368 closed Bug / Defect (fixed)

--auth-user-pass does not null-terminate strings from console input via systemd-ask-password

Reported by: harzaf Owned by: dazo
Priority: major Milestone: release 2.3.11
Component: Generic / unclassified Version: OpenVPN 2.3.2 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords:
Cc:

Description

Steps to Reproduce

  • Linux distro with systemd (testing on Arch Linux x64)
  • OpenVPN 2.3.2 running as a client
  • --auth-user-pass (to read in from console; no file specified)
  • Enter a username of maximum length (USER_PASS_LEN in misc.h)
  • Enter any password
  • Observe in a debugger/printf that Openvpn has the username=username + password (because the username string is not null terminated)
  • This causes a problem accessing at least one VPN provider which uses usernames of length 128 when the binary has USER_PASS_LEN as 128. (Eg Cryptostorm)

Error

In console.c:

 get_console_input_systemd (const char *prompt, const bool echo, char *input, const int capacity)
{
  ...

  CLEAR (*input);
  if (read (std_out, input, capacity) != 0)
    {
       chomp (input);
       ret = true;
    }
  ...
}

The read() fills the entire input buffer, which is then no longer null-terminated string.

Fix

The ideal fix should:

  1. Ensure the input is always null-terminated
  2. Warn the user if the input is too long instead of silently using wrong username/password

In console.c

get_console_input_systemd (const char *prompt, const bool echo, char *input, const int capacity)
{
  ssize_t bytes;

  ...

  CLEAR (*input); 
  
  bytes = read (std_out, input, capacity);
  
  if (bytes == capacity)
    {
      /* Warn the user instead of silently truncating input */
      msg (M_FATAL, "Input too long for buffer.", prompt);
    }
  else if (bytes != 0)
    {
       chomp (input);
       ret = true;
    }
...
}

Attachments (1)

console.c.patch (1000 bytes) - added by harzaf 4 years ago.
Patch for console.c

Download all attachments as: .zip

Change History (4)

Changed 4 years ago by harzaf

Patch for console.c

comment:1 Changed 3 years ago by samuli

  • Owner set to dazo
  • Status changed from new to assigned

Assigning to dazo because systemd.

comment:2 Changed 3 years ago by samuli

  • Milestone set to release 2.5

comment:3 Changed 13 months ago by dazo

  • Milestone changed from release 2.5 to release 2.3.11
  • Resolution set to fixed
  • Status changed from assigned to closed

This should be fixed since v2.3.11 and master+2.4_alpha1

commit 17b22f9da909f0193dff30860b45673d9cd6c509
Author: Selva Nair <selva.nair@gmail.com>
Date:   Wed Apr 13 23:53:33 2016 -0400

    Ensure input read using systemd-ask-password is null terminated
    
    Also properly check the return value of read() and leave room
    for termination.
    Fixes junk data occasionally seen in strings read through systemd.
    
    Signed-off-by: Selva Nair <selva.nair@gmail.com>
    Acked-by: Gert Doering <gert@greenie.muc.de>
    Message-Id: <1460606013-4983-1-git-send-email-selva.nair@gmail.com>
    URL: http://article.gmane.org/gmane.network.openvpn.devel/11437
    Signed-off-by: Gert Doering <gert@greenie.muc.de>
    (cherry picked from commit d09fbf958f1c0b15372b3e87d784ae666b91a96b)

In addition the systemd part got revamped as well, in master/2.4_alpha

commit 3280d8c8f3583d03fed923837447ef45c8f83d52
Author: David Sommerseth <davids@openvpn.net>
Date:   Fri Aug 12 12:57:25 2016 +0200

    Re-implement the systemd support using the new query user API
    
    This provides exactly the same systemd functionality which existed
    before the query user infrastructure got implemented.
    
      [v5 - Ensure NULL termination fix in d09fbf958f1c is included ]
    
      [v4 - change disapproved &= syntax ]
    
      [v3 - Remove QUERY_USER_EXEC_ALTERNATIVE macro, simplify
            alternatives definition directly in console.h.  For
            now only depend on ENABLE_SYSTEMD]
    
      [v2 - Removed the QUERY_USER_FOREACH macro]
    
    Signed-off-by: David Sommerseth <davids@openvpn.net>
    Acked-by: Selva Nair <selva.nair@gmail.com>
    Message-Id: 1470999445-4288-1-git-send-email-davids@openvpn.net
    URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12424.html

This commit explicitly ensures the former patch is fixed in the new implementation as well.

Note: See TracTickets for help on using tickets.