Re: Fix for /usr/ports/devel/startup-notification/patches/patch-libsn_sn-util_c

[Theo de Raadt]

-  strcpy (s, str);

Yes, the old code is totally wrong for using strcpy.

+  strncpy (s, str, sizeof(s));

And whoever tried to fix it, made it even worse.  Now it only copies
either 4 bytes, 8 bytes, or the size of the destination buffer if it
is not a pointer (and it is a pointer, isn't it).  Heck, they might as
well have put a value 1 in there, for all that diff is worth.

+  strncpy (s, str, strlen (str) + 1);

Ah, but this diff is 100% the same as the original strcpy().  Think
about it.

If that is the best anyone can do, amateurish "buffer overflow fixing"
which makes it worse or doesn't fix it, then they should not change
the code.  There is an alternative.

Some people should instead try the alternative:

Don't try to fix it, but yell at the authors about the
problem repeatedly.

That's something even amateurs can do.  If you can spot it, you can
always get it fixed correctly that way.  But don't rely on even OpenBSD
developers (or ports contributers) to get this this right all the time.
The responsibility always lies with the official maintainer...

Fix for /usr/ports/devel/startup-notification/patches/patch-libsn_sn-util_c

[Michael Mitton]

I found a problem with a patch in startup-notification.  The strncpy
statements were not copying the correct amount of data due to using
sizeof() instead of strlen() values.  Old patch and fixed patch below.

-Michael


Old Patch

$OpenBSD: patch-libsn_sn-util_c,v 1.1 2005/05/25 23:53:37 marcm Exp $
--- libsn/sn-util.c.orig   Wed May 25 16:35:54 2005
+++ libsn/sn-util.c  Wed May 25 16:37:28 2005
@@ -257,7 +257,7 @@ sn_internal_strdup (const char *str)
  char *s;

  s = sn_malloc (strlen (str) + 1);
-  strcpy (s, str);
+  strncpy (s, str, sizeof(s));

  return s;
}
@@ -376,6 +376,6 @@ sn_internal_append_to_string (char
  *append_to = sn_realloc (*append_to, *current_len + len + 1);

  end = *append_to + *current_len;
-  strcpy (end, append);
+  strncpy (end, append, sizeof(end));
  *current_len = *current_len + len;
}


New Patch

$OpenBSD: patch-libsn_sn-util_c,v 1.1 2005/05/25 23:53:37 marcm Exp $
--- libsn/sn-util.c.orig   Wed May 25 16:35:54 2005
+++ libsn/sn-util.c  Wed May 25 16:37:28 2005
@@ -257,7 +257,7 @@ sn_internal_strdup (const char *str)
  char *s;

  s = sn_malloc (strlen (str) + 1);
-  strcpy (s, str);
+  strncpy (s, str, strlen (str) + 1);

  return s;
}
@@ -376,6 +376,6 @@ sn_internal_append_to_string (char
  *append_to = sn_realloc (*append_to, *current_len + len + 1);

  end = *append_to + *current_len;
-  strcpy (end, append);
+  strncpy (end, append, len + 1);
  *current_len = *current_len + len;
}