Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The command line arguments '-u user:password' are not hidden #9128

Closed
bagder opened this issue Jul 9, 2022 Discussed in #9127 · 0 comments
Closed

The command line arguments '-u user:password' are not hidden #9128

bagder opened this issue Jul 9, 2022 Discussed in #9127 · 0 comments

Comments

@bagder
Copy link
Member

bagder commented Jul 9, 2022

Discussed in #9127

Originally posted by LitterWhite July 9, 2022
I seem to have found a bug.

When starting curl, I pass username and password via command line, like this:

curl -u user:password ...

Then check the cmdline of curl by ps and expect user name and password are hidden,like this:

curl -u              ...

But they are not.

I check the code and found the arg of 'user:password' will be clean at getparameter()

src/tool_getparam.c: getparameter()

case 'u':
      /* user:password  */
      GetStr(&config->userpwd, nextarg);
      cleanarg(nextarg);
      break;

src/tool_paramhlp.c: cleanarg()

void cleanarg(char *str)
{
#ifdef HAVE_WRITABLE_ARGV
  /* now that GetStr has copied the contents of nextarg, wipe the next
   * argument out so that the username:password isn't displayed in the
   * system process list */
  if(str) {
    size_t len = strlen(str);
    memset(str, ' ', len);
  }
#else
  (void)str;
#endif
}

However, the pointer 'nextarg' pass to cleanarg is not the argv[] from main(argc, argv), it is copyed from argv[] at parse_args()

src/tool_getparam.c: parse_args()

        char *nextarg = (i < (argc - 1))
          ? curlx_convert_tchar_to_UTF8(argv[i + 1])
          : NULL;

        result = getparameter(orig_opt, nextarg, &passarg, global, config);

        curlx_unicodefree(nextarg);

lib/curl_multibyte.h

#if defined(UNICODE) && defined(WIN32)

#define curlx_convert_UTF8_to_tchar(ptr) curlx_convert_UTF8_to_wchar((ptr))
#define curlx_convert_tchar_to_UTF8(ptr) curlx_convert_wchar_to_UTF8((ptr))

typedef union {
  unsigned short       *tchar_ptr;
  const unsigned short *const_tchar_ptr;
  unsigned short       *tbyte_ptr;
  const unsigned short *const_tbyte_ptr;
} xcharp_u;

#else

#define curlx_convert_UTF8_to_tchar(ptr) (strdup)(ptr)
#define curlx_convert_tchar_to_UTF8(ptr) (strdup)(ptr)

Thus, the user name and password in argv are not cleaned up.

The last version is this:

src/tool_getparam.c: parse_args()

        char *nextarg = (i < (argc - 1)) ? argv[i + 1] : NULL;
        result = getparameter(flag, nextarg, &passarg, global, config);

it was changed at commit 9e5669f

Maybe my understanding of the code is wrong, looking forward to a reply

bagder added a commit that referenced this issue Jul 10, 2022
Regression since 9e5669f.

Make sure the "cleaning" of command line arguments are done on the
original argv[] pointers.

Reported-by: Litter White
Fixes #9128
bagder added a commit that referenced this issue Jul 10, 2022
Regression since 9e5669f.

Make sure the "cleaning" of command line arguments is done on the
original argv[] pointers.

Reported-by: Litter White
Fixes #9128
Closes #9130
@bagder bagder closed this as completed in bf7e887 Jul 10, 2022
jay added a commit to jay/curl that referenced this issue Jul 14, 2022
Follow-up to bf7e887 which fixed cleaning sensitive arguments from the
command line in Windows. That fix (if it works) only works for Windows
multibyte builds (char) and not Windows Unicode builds (wchar_t).

Ref: curl#9128

Closes #xxxx
jay added a commit to jay/curl that referenced this issue Jul 14, 2022
Follow-up to bf7e887 which fixed cleaning sensitive arguments from the
command line in Windows. That fix (if it works) only works for Windows
multibyte builds (char) and not Windows Unicode builds (wchar_t).

Ref: curl#9128

Closes #xxxx
bagder added a commit that referenced this issue Aug 30, 2022
This reverts commit bf7e887.

Should repair test 2056 2057 but brings back issue #9128

Fixes #9397
bagder added a commit that referenced this issue Aug 30, 2022
Follow-up to bf7e887

The previous fix for #9128 was incomplete and caused #9397.

Fixes #9397
bagder added a commit that referenced this issue Aug 30, 2022
Follow-up to bf7e887

The previous fix for #9128 was incomplete and caused #9397.

Fixes #9397
bagder added a commit that referenced this issue Aug 30, 2022
Follow-up to bf7e887

The previous fix for #9128 was incomplete and caused #9397.

Fixes #9397
Closes #9399
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

1 participant