You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by tr...@apache.org on 2004/11/30 15:41:33 UTC
svn commit: r107007 - /apr/apr/trunk/CHANGES /apr/apr/trunk/include/apr_lib.h /apr/apr/trunk/passwd/apr_getpass.c
Author: trawick
Date: Tue Nov 30 06:41:31 2004
New Revision: 107007
URL: http://svn.apache.org/viewcvs?view=rev&rev=107007
Log:
apr_password_get(): Fix the check for buffer overflow.
The input buffer had already been cleared by the
time the length of the input buffer was checked,
so overflow was never reported.
Add a comment about the length checking to the docs.
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/include/apr_lib.h
apr/apr/trunk/passwd/apr_getpass.c
Modified: apr/apr/trunk/CHANGES
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/CHANGES?view=diff&rev=107007&p1=apr/apr/trunk/CHANGES&r1=107006&p2=apr/apr/trunk/CHANGES&r2=107007
==============================================================================
--- apr/apr/trunk/CHANGES (original)
+++ apr/apr/trunk/CHANGES Tue Nov 30 06:41:31 2004
@@ -32,6 +32,8 @@
Changes for APR 1.0.1
+ *) apr_password_get(): Fix the check for buffer overflow. [Jeff Trawick]
+
*) Fix HUP return codes in pollset when using KQueue.
[Paul Querna]
Modified: apr/apr/trunk/include/apr_lib.h
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/include/apr_lib.h?view=diff&rev=107007&p1=apr/apr/trunk/include/apr_lib.h&r1=107006&p2=apr/apr/trunk/include/apr_lib.h&r2=107007
==============================================================================
--- apr/apr/trunk/include/apr_lib.h (original)
+++ apr/apr/trunk/include/apr_lib.h Tue Nov 30 06:41:31 2004
@@ -168,6 +168,8 @@
* @param prompt The prompt to display
* @param pwbuf Buffer to store the password
* @param bufsize The length of the password buffer.
+ * @remark If the password entered must be truncated to fit in
+ * the provided buffer, APR_ENAMETOOLONG will be returned.
*/
APR_DECLARE(apr_status_t) apr_password_get(const char *prompt, char *pwbuf,
apr_size_t *bufsize);
Modified: apr/apr/trunk/passwd/apr_getpass.c
Url: http://svn.apache.org/viewcvs/apr/apr/trunk/passwd/apr_getpass.c?view=diff&rev=107007&p1=apr/apr/trunk/passwd/apr_getpass.c&r1=107006&p2=apr/apr/trunk/passwd/apr_getpass.c&r2=107007
==============================================================================
--- apr/apr/trunk/passwd/apr_getpass.c (original)
+++ apr/apr/trunk/passwd/apr_getpass.c Tue Nov 30 06:41:31 2004
@@ -219,12 +219,14 @@
#else
char *pw_got = getpass(prompt);
#endif
+ apr_status_t rv = APR_SUCCESS;
+
if (!pw_got)
return APR_EINVAL;
- apr_cpystrn(pwbuf, pw_got, *bufsiz);
- memset(pw_got, 0, strlen(pw_got));
if (strlen(pw_got) >= *bufsiz) {
- return APR_ENAMETOOLONG;
+ rv = APR_ENAMETOOLONG;
}
- return APR_SUCCESS;
+ apr_cpystrn(pwbuf, pw_got, *bufsiz);
+ memset(pw_got, 0, strlen(pw_got));
+ return rv;
}
Re: svn commit: r107007 - /apr/apr/trunk/CHANGES /apr/apr/trunk/include/apr_lib.h /apr/apr/trunk/passwd/apr_getpass.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, 30 Nov 2004 10:26:31 -0500 (EST), Cliff Woolley
<jw...@virginia.edu> wrote:
> On Tue, 30 Nov 2004, Jeff Trawick wrote:
>
>
>
> > > * @param pwbuf Buffer to store the password
> > > * @param bufsize The length of the password buffer.
> > > + * @remark If the password entered must be truncated to fit in
> > > + * the provided buffer, APR_ENAMETOOLONG will be returned.
> > > */
> > > APR_DECLARE(apr_status_t) apr_password_get(const char *prompt, char *pwbuf,
> > > apr_size_t *bufsize);
> >
> > another disturbance: we force caller to go to the trouble to pass by
> > address, but we don't update the size on output to indicate either the
> > number of bytes stored or the number of bytes needed; shrug or "fix"?
>
> By number of bytes stored, I assume you mean bytes stored in the non-error
> case, since bytes stored in the error case would just be bufsize. Right?
bufsize - 1, since I don't think we'd count the '\0'
>
> If that's the case, then it's maybe worth fixing but I'm not sure I care.
> How many bytes would be needed seems to make somewhat more sense as being
> a useful value, but do we have any other functions that work that way?
agreed on that being more useful
.
apr_strftime() is a function which has "apr_size_t *" to report the
length of the returned string, but on overflow it seems to return 0.
(apr_strftime() defers to strftime() to do the dirty work, and
strftime() doc says
If the
total number of resulting characters including the terminat-
ing null character is more than maxsize, strftime() returns
0 and the contents of the array are indeterminate.
)
.
an ugly mess all around; getpass() itself can truncate the user's
input but not report to us that it truncated (try entering more than 8
chars on HP-UX), so we can't accurately report truncation unless we
modify our getpass() implementation to report that, and always use our
own instead of the system getpass()
punt
Re: svn commit: r107007 - /apr/apr/trunk/CHANGES /apr/apr/trunk/include/apr_lib.h
/apr/apr/trunk/passwd/apr_getpass.c
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 30 Nov 2004, Jeff Trawick wrote:
> > * @param pwbuf Buffer to store the password
> > * @param bufsize The length of the password buffer.
> > + * @remark If the password entered must be truncated to fit in
> > + * the provided buffer, APR_ENAMETOOLONG will be returned.
> > */
> > APR_DECLARE(apr_status_t) apr_password_get(const char *prompt, char *pwbuf,
> > apr_size_t *bufsize);
>
> another disturbance: we force caller to go to the trouble to pass by
> address, but we don't update the size on output to indicate either the
> number of bytes stored or the number of bytes needed; shrug or "fix"?
By number of bytes stored, I assume you mean bytes stored in the non-error
case, since bytes stored in the error case would just be bufsize. Right?
If that's the case, then it's maybe worth fixing but I'm not sure I care.
How many bytes would be needed seems to make somewhat more sense as being
a useful value, but do we have any other functions that work that way?
Re: svn commit: r107007 - /apr/apr/trunk/CHANGES /apr/apr/trunk/include/apr_lib.h /apr/apr/trunk/passwd/apr_getpass.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Wed, 1 Dec 2004 12:04:14 +0000, Joe Orton <jo...@redhat.com> wrote:
> On Tue, Nov 30, 2004 at 10:11:02AM -0500, Jeff Trawick wrote:
>
>
> > On 30 Nov 2004 14:41:33 -0000, trawick@apache.org <tr...@apache.org> wrote:
> > > apr_password_get(): Fix the check for buffer overflow.
> > > --- apr/apr/trunk/include/apr_lib.h (original)
> > > +++ apr/apr/trunk/include/apr_lib.h Tue Nov 30 06:41:31 2004
> > > @@ -168,6 +168,8 @@
> > > * @param prompt The prompt to display
> > > * @param pwbuf Buffer to store the password
> > > * @param bufsize The length of the password buffer.
> > > + * @remark If the password entered must be truncated to fit in
> > > + * the provided buffer, APR_ENAMETOOLONG will be returned.
> > > */
> > > APR_DECLARE(apr_status_t) apr_password_get(const char *prompt, char *pwbuf,
> > > apr_size_t *bufsize);
> >
> > another disturbance: we force caller to go to the trouble to pass by
> > address, but we don't update the size on output to indicate either the
> > number of bytes stored or the number of bytes needed; shrug or "fix"?
>
> I think "shrug" - any callers might as well call strlen() than rely on a
> future version of the function which updates *bufsize. I've updated to
> the docco to fix that in stone, hope that's OK.
+1
Re: svn commit: r107007 - /apr/apr/trunk/CHANGES /apr/apr/trunk/include/apr_lib.h /apr/apr/trunk/passwd/apr_getpass.c
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Nov 30, 2004 at 10:11:02AM -0500, Jeff Trawick wrote:
> On 30 Nov 2004 14:41:33 -0000, trawick@apache.org <tr...@apache.org> wrote:
> > apr_password_get(): Fix the check for buffer overflow.
> > --- apr/apr/trunk/include/apr_lib.h (original)
> > +++ apr/apr/trunk/include/apr_lib.h Tue Nov 30 06:41:31 2004
> > @@ -168,6 +168,8 @@
> > * @param prompt The prompt to display
> > * @param pwbuf Buffer to store the password
> > * @param bufsize The length of the password buffer.
> > + * @remark If the password entered must be truncated to fit in
> > + * the provided buffer, APR_ENAMETOOLONG will be returned.
> > */
> > APR_DECLARE(apr_status_t) apr_password_get(const char *prompt, char *pwbuf,
> > apr_size_t *bufsize);
>
> another disturbance: we force caller to go to the trouble to pass by
> address, but we don't update the size on output to indicate either the
> number of bytes stored or the number of bytes needed; shrug or "fix"?
I think "shrug" - any callers might as well call strlen() than rely on a
future version of the function which updates *bufsize. I've updated to
the docco to fix that in stone, hope that's OK.
joe
Re: svn commit: r107007 - /apr/apr/trunk/CHANGES /apr/apr/trunk/include/apr_lib.h /apr/apr/trunk/passwd/apr_getpass.c
Posted by Jeff Trawick <tr...@gmail.com>.
On 30 Nov 2004 14:41:33 -0000, trawick@apache.org <tr...@apache.org> wrote:
> apr_password_get(): Fix the check for buffer overflow.
> --- apr/apr/trunk/include/apr_lib.h (original)
> +++ apr/apr/trunk/include/apr_lib.h Tue Nov 30 06:41:31 2004
> @@ -168,6 +168,8 @@
> * @param prompt The prompt to display
> * @param pwbuf Buffer to store the password
> * @param bufsize The length of the password buffer.
> + * @remark If the password entered must be truncated to fit in
> + * the provided buffer, APR_ENAMETOOLONG will be returned.
> */
> APR_DECLARE(apr_status_t) apr_password_get(const char *prompt, char *pwbuf,
> apr_size_t *bufsize);
another disturbance: we force caller to go to the trouble to pass by
address, but we don't update the size on output to indicate either the
number of bytes stored or the number of bytes needed; shrug or "fix"?