{"thread":{"id":"1828","subject":"sprintf security holes?","startedAt":"2005-09-16T14:56:05Z","lastAt":"2005-09-19T12:42:22Z","messageCount":5,"participants":["Randal L. Schwartz","Morten Welinder","Peter Eriksen","Junio C Hamano","Sven Verdoolaege"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"8681","messageId":"86zmqd5aey.fsf@blue.stonehenge.com","threadId":"1828","inReplyTo":null,"subject":"sprintf security holes?","fromName":"Randal L. Schwartz","fromEmail":"merlyn@stonehenge.com","sentAt":"2005-09-16T14:56:05Z","receivedAt":"2005-09-16T14:56:05Z","isPatch":false,"sender":{"key":"merlyn@stonehenge.com","avatar":"https://gravatar.com/avatar/dc528d210743ff0333e6213f9ee7b33b23f1b7bc1f3c5a8c2d819074ecd7ab19?d=mp&s=160"},"body":"\nAs I was trying to (unsuccessfully) compile git for OpenBSD, I noticed\na number of occurances of sprintf(), because OpenBSD thoughtfully\nyells at programmers for using that.\n\nSince sprintf() can lead to buffer overflows from unprotected user\ndata, and you want to use git in server situtations, wouldn't it be\nprudent to eliminate those in some near-ish timeframe?\n\n-- \nRandal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095\n<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>\nPerl/Unix/security consulting, Technical writing, Comedy, etc. etc.\nSee PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!\n"},{"id":"8685","messageId":"118833cc05091609111fd4d2d8@mail.gmail.com","threadId":"1828","inReplyTo":"86zmqd5aey.fsf@blue.stonehenge.com","subject":"Re: sprintf security holes?","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2005-09-16T16:11:16Z","receivedAt":"2005-09-16T16:11:16Z","isPatch":false,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"> Since sprintf() can lead to buffer overflows from unprotected user\n> data, and you want to use git in server situtations, wouldn't it be\n> prudent to eliminate those in some near-ish timeframe?\n\nI assume you mean make them into snprintf calls.\n\nI don't think that really buys you anything, i.e., there would still be far\ntoo many places where character pointers are derefenced and\nassigned to.  You would probably have to create a small (and thus\nauditable) generic strings library and do all string creation within\nthat.  That is certainly feasible, but evidently not Linus style.\n\nThere really is nothing wrong with sprintf that couldn't be said to be\nwrong with snprintf, strcpy, strncpy, *p++ = 0, etc.  If you don't have\nthe right amount of memory allocated, you lose.  (Yes, that goes for\nthe \"n\" versions too, although in many cases you could syntactically\ncheck that a \"sizeof\" was used.  Then you just get to worry whether\nrandom truncation introduced other security problems.)\n\nMorten\n"},{"id":"8732","messageId":"20050916213627.GA19867@ebar091.ebar.dtu.dk","threadId":"1828","inReplyTo":"86zmqd5aey.fsf@blue.stonehenge.com","subject":"[Was Re: sprintf security holes?] Building on OpenBSD","fromName":"Peter Eriksen","fromEmail":"s022018@student.dtu.dk","sentAt":"2005-09-16T21:36:27Z","receivedAt":"2005-09-16T21:36:27Z","isPatch":false,"sender":{"key":"s022018@student.dtu.dk","avatar":null},"body":"On Fri, Sep 16, 2005 at 07:56:05AM -0700, Randal L. Schwartz wrote:\n> \n> As I was trying to (unsuccessfully) compile git for OpenBSD, I noticed\n> a number of occurances of sprintf(), because OpenBSD thoughtfully\n> yells at programmers for using that.\n\nI've got git mostly working on OpenBSD.  Here is what I changed:\n\ndiff --git a/Makefile b/Makefile\n--- a/Makefile\n+++ b/Makefile\n@@ -87,7 +87,7 @@ SCRIPT_PYTHON = \\\n\n # The ones that do not have to link with lcrypto nor lz.\n SIMPLE_PROGRAMS = \\\n-       git-get-tar-commit-id git-mailinfo git-mailsplit git-stripspace\n        \\\n+       git-get-tar-commit-id git-mailsplit git-stripspace \\\n        git-daemon git-var\n\n # ... and all the rest\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -50,7 +50,7 @@ test_expect_success \\\n\n test_expect_success \\\n     'validate file modification time' \\\n-    'TZ=GMT tar tvf b.tar a/a |\n+    'TZ=GMT gtar tvf b.tar a/a |\n      awk \\{print\\ \\$4,\\ \\(length\\(\\$5\\)\\<7\\)\\ ?\\ \\$5\\\":00\\\"\\ :\\ \\$5\\} \\\n      >b.mtime &&\n      echo \"2005-05-27 22:00:00\" >expected.mtime &&\n \n\nThe two issues showing up are\n1) missing strcasestr() which is used in mailinfo.c\n2) OpenBSD tar tvf doesn't format the date as we expect.\n\nThere were talks about making a directory compat/ with system \nspecific code, where an implementation of strcasestr.c could go?\n\nWith those changes all I did to compile was: \n\ngmake CFLAGS+='-I/usr/local/include/ -L/usr/local/lib'\ngmake test\ngmake install\n\n\nRegards,\n\nPeter\n"},{"id":"8839","messageId":"7voe6pv6zz.fsf@assigned-by-dhcp.cox.net","threadId":"1828","inReplyTo":"86zmqd5aey.fsf@blue.stonehenge.com","subject":"Re: sprintf security holes?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-19T07:42:08Z","receivedAt":"2005-09-19T07:42:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"merlyn@stonehenge.com (Randal L. Schwartz) writes:\n\n> Since sprintf() can lead to buffer overflows from unprotected user\n> data, and you want to use git in server situtations, wouldn't it be\n> prudent to eliminate those in some near-ish timeframe?\n\nI do not have any objection for a code audit, but what I do not\nparticularly like about the above statement is what to do after\nyou audited and know your sprintf() would not overrun any of\nyour allocated buffers.  I suspect OpenBSD will keep complaining\njust because sprintf() is used.\n\nRewriting them all to use snprintf() is probably not a good\nenough answer.  Later somebody will make mistakes while changing\nour code, and if the modified format string (or any other change\nthat affects the condition under which snprintf() is called)\nwould now generate a string longer than allotted buffer, then\nwhat happens?  Does OpenBSD nicely complain again, or keep mum\nabout the problem because it is happy that we are using\nsnprintf()?\n\nWe _might_ be protected from buffer overrun errors [*1*], but we\nwould then end up operating on a truncated data -- which would\nlead to an equally unexpected behaviour.  I do not think there\nis any guarantee that working with unexpectedly truncated data\nleads to less severe security problems than buffer overruns with\ncareless sprintf().\n\n\n[Footnotes]\n\n*1* I do not think even that is guaranteed -- does OpenBSD\ncompare the xmalloc()'ed buffer length passed to snprintf\nagainst the size limit?  What if we pass a statically allocated\nbuffer as a pointer to another function that uses snprintf?\nDoes it follow the flow of the data in such a way to take\nnotice?\n"},{"id":"8851","messageId":"20050919124222.GD15165MdfPADPa@greensroom.kotnet.org","threadId":"1828","inReplyTo":"7voe6pv6zz.fsf@assigned-by-dhcp.cox.net","subject":"Re: sprintf security holes?","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2005-09-19T12:42:22Z","receivedAt":"2005-09-19T12:42:22Z","isPatch":false,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Mon, Sep 19, 2005 at 12:42:08AM -0700, Junio C Hamano wrote:\n> We _might_ be protected from buffer overrun errors [*1*], but we\n> would then end up operating on a truncated data -- which would\n> lead to an equally unexpected behaviour.  I do not think there\n> is any guarantee that working with unexpectedly truncated data\n> leads to less severe security problems than buffer overruns with\n> careless sprintf().\n\nBut you can _check_ whether the data has been truncated.\nJust look at the return value of snprintf.\n\nskimo\n"}]}