Re: ssh-add(1): fix NULL in fprintf
On Fri, 17 Jun 2022 at 04:49, Martin Vahlensieck wrote: > ping, diff attached Applied, thanks. -- Darren Tucker (dtucker at dtucker.net) GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860 37F4 9357 ECEF 11EA A6FA (new) Good judgement comes with experience. Unfortunately, the experience usually comes from bad judgement.
Re: ssh-add(1): fix NULL in fprintf
ping, diff attached
On Mon, May 16, 2022 at 09:21:42PM +0200, Martin Vahlensieck wrote:
> Hi
>
> What's the status on this? Anthing required from my side? I have
> reattached the patch (with the changes Theo suggested).
>
> Best,
>
> Martin
>
> On Mon, May 09, 2022 at 08:39:38PM +0200, Martin Vahlensieck wrote:
> > On Mon, May 09, 2022 at 10:42:29AM -0600, Theo de Raadt wrote:
> > > Martin Vahlensieck wrote:
> > >
> > > > if (!qflag) {
> > > > - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > > > - sshkey_type(key), comment);
> > > > + fprintf(stderr, "Identity removed: %s %s%s%s%s\n", path,
> > > > + sshkey_type(key), comment ? " (" : "",
> > > > + comment ? comment : "", comment ? ")" : "");
> > >
> > > this is probably better as something like
> > >
> > > > - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > > > - sshkey_type(key), comment ? comment : "no comment");
> > >
> > > Which has a minor ambiguity, but probably harms noone.
> > >
> >
> > Index: ssh-add.c
> > ===
> > RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
> > retrieving revision 1.165
> > diff -u -p -r1.165 ssh-add.c
> > --- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
> > +++ ssh-add.c 9 May 2022 18:36:54 -
> > @@ -118,7 +118,7 @@ delete_one(int agent_fd, const struct ss
> > }
> > if (!qflag) {
> > fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > - sshkey_type(key), comment);
> > + sshkey_type(key), comment ? comment : "no comment");
> > }
> > return 0;
> > }
> > @@ -392,7 +392,7 @@ add_file(int agent_fd, const char *filen
> > certpath, filename);
> > sshkey_free(cert);
> > goto out;
> > - }
> > + }
> >
> > /* Graft with private bits */
> > if ((r = sshkey_to_certified(private)) != 0) {
>
> Index: ssh-add.c
> ===
> RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
> retrieving revision 1.165
> diff -u -p -r1.165 ssh-add.c
> --- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
> +++ ssh-add.c 9 May 2022 18:36:54 -
> @@ -118,7 +118,7 @@ delete_one(int agent_fd, const struct ss
> }
> if (!qflag) {
> fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> - sshkey_type(key), comment);
> + sshkey_type(key), comment ? comment : "no comment");
> }
> return 0;
> }
> @@ -392,7 +392,7 @@ add_file(int agent_fd, const char *filen
> certpath, filename);
> sshkey_free(cert);
> goto out;
> - }
> + }
>
> /* Graft with private bits */
> if ((r = sshkey_to_certified(private)) != 0) {
>
Index: ssh-add.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
retrieving revision 1.165
diff -u -p -r1.165 ssh-add.c
--- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
+++ ssh-add.c 9 May 2022 18:36:54 -
@@ -118,7 +118,7 @@ delete_one(int agent_fd, const struct ss
}
if (!qflag) {
fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
- sshkey_type(key), comment);
+ sshkey_type(key), comment ? comment : "no comment");
}
return 0;
}
@@ -392,7 +392,7 @@ add_file(int agent_fd, const char *filen
certpath, filename);
sshkey_free(cert);
goto out;
- }
+ }
/* Graft with private bits */
if ((r = sshkey_to_certified(private)) != 0) {
Re: ssh-add(1): fix NULL in fprintf
Hi
What's the status on this? Anthing required from my side? I have
reattached the patch (with the changes Theo suggested).
Best,
Martin
On Mon, May 09, 2022 at 08:39:38PM +0200, Martin Vahlensieck wrote:
> On Mon, May 09, 2022 at 10:42:29AM -0600, Theo de Raadt wrote:
> > Martin Vahlensieck wrote:
> >
> > > if (!qflag) {
> > > - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > > - sshkey_type(key), comment);
> > > + fprintf(stderr, "Identity removed: %s %s%s%s%s\n", path,
> > > + sshkey_type(key), comment ? " (" : "",
> > > + comment ? comment : "", comment ? ")" : "");
> >
> > this is probably better as something like
> >
> > > - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > > - sshkey_type(key), comment ? comment : "no comment");
> >
> > Which has a minor ambiguity, but probably harms noone.
> >
>
> Index: ssh-add.c
> ===
> RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
> retrieving revision 1.165
> diff -u -p -r1.165 ssh-add.c
> --- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
> +++ ssh-add.c 9 May 2022 18:36:54 -
> @@ -118,7 +118,7 @@ delete_one(int agent_fd, const struct ss
> }
> if (!qflag) {
> fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> - sshkey_type(key), comment);
> + sshkey_type(key), comment ? comment : "no comment");
> }
> return 0;
> }
> @@ -392,7 +392,7 @@ add_file(int agent_fd, const char *filen
> certpath, filename);
> sshkey_free(cert);
> goto out;
> - }
> + }
>
> /* Graft with private bits */
> if ((r = sshkey_to_certified(private)) != 0) {
Index: ssh-add.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
retrieving revision 1.165
diff -u -p -r1.165 ssh-add.c
--- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
+++ ssh-add.c 9 May 2022 18:36:54 -
@@ -118,7 +118,7 @@ delete_one(int agent_fd, const struct ss
}
if (!qflag) {
fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
- sshkey_type(key), comment);
+ sshkey_type(key), comment ? comment : "no comment");
}
return 0;
}
@@ -392,7 +392,7 @@ add_file(int agent_fd, const char *filen
certpath, filename);
sshkey_free(cert);
goto out;
- }
+ }
/* Graft with private bits */
if ((r = sshkey_to_certified(private)) != 0) {
Re: ssh-add(1): fix NULL in fprintf
On Mon, May 09, 2022 at 10:42:29AM -0600, Theo de Raadt wrote:
> Martin Vahlensieck wrote:
>
> > if (!qflag) {
> > - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > - sshkey_type(key), comment);
> > + fprintf(stderr, "Identity removed: %s %s%s%s%s\n", path,
> > + sshkey_type(key), comment ? " (" : "",
> > + comment ? comment : "", comment ? ")" : "");
>
> this is probably better as something like
>
> > - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> > - sshkey_type(key), comment ? comment : "no comment");
>
> Which has a minor ambiguity, but probably harms noone.
>
Index: ssh-add.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
retrieving revision 1.165
diff -u -p -r1.165 ssh-add.c
--- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
+++ ssh-add.c 9 May 2022 18:36:54 -
@@ -118,7 +118,7 @@ delete_one(int agent_fd, const struct ss
}
if (!qflag) {
fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
- sshkey_type(key), comment);
+ sshkey_type(key), comment ? comment : "no comment");
}
return 0;
}
@@ -392,7 +392,7 @@ add_file(int agent_fd, const char *filen
certpath, filename);
sshkey_free(cert);
goto out;
- }
+ }
/* Graft with private bits */
if ((r = sshkey_to_certified(private)) != 0) {
Re: ssh-add(1): fix NULL in fprintf
Martin Vahlensieck wrote:
> if (!qflag) {
> - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> - sshkey_type(key), comment);
> + fprintf(stderr, "Identity removed: %s %s%s%s%s\n", path,
> + sshkey_type(key), comment ? " (" : "",
> + comment ? comment : "", comment ? ")" : "");
this is probably better as something like
> - fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
> - sshkey_type(key), comment ? comment : "no comment");
Which has a minor ambiguity, but probably harms noone.
ssh-add(1): fix NULL in fprintf
Hi
When removing an identity from the agent using the private key file,
ssh-add first tries to find the public key file. If that fails,
it loads the public key from the private key file, but no comment
is loaded. This means comment is NULL when it is used inside
delete_one to print `Identity removed: ...'
Below is a diff which only prints the braces and the comment if it
is not NULL. Something similar is done in ssh-keygen.c line
2423-2425.
So with the following setup:
$ ssh-keygen -t ed25519 -f demo -C demo -N ''
$ mv demo.pub demo_pub
$ ssh-add demo
Identity added: demo (demo)
Before:
$ ssh-add -d demo
Identity removed: demo ED25519 ((null))
$ tail -n 1 /var/log/messages
May 9 18:15:53 demo ssh-add: vfprintf %s NULL in "Identity removed: %s %s
(%s) "
After:
$ ssh-add -d demo
Identity removed: demo ED25519
Best,
Martin
P.S.: While here remove a trailing space as well.
Index: ssh-add.c
===
RCS file: /cvs/src/usr.bin/ssh/ssh-add.c,v
retrieving revision 1.165
diff -u -p -r1.165 ssh-add.c
--- ssh-add.c 4 Feb 2022 02:49:17 - 1.165
+++ ssh-add.c 9 May 2022 16:04:14 -
@@ -117,8 +117,9 @@ delete_one(int agent_fd, const struct ss
return r;
}
if (!qflag) {
- fprintf(stderr, "Identity removed: %s %s (%s)\n", path,
- sshkey_type(key), comment);
+ fprintf(stderr, "Identity removed: %s %s%s%s%s\n", path,
+ sshkey_type(key), comment ? " (" : "",
+ comment ? comment : "", comment ? ")" : "");
}
return 0;
}
@@ -392,7 +393,7 @@ add_file(int agent_fd, const char *filen
certpath, filename);
sshkey_free(cert);
goto out;
- }
+ }
/* Graft with private bits */
if ((r = sshkey_to_certified(private)) != 0) {
