* [9fans] lpdaemon @ 2013-06-05 11:38 yaroslav 2013-06-05 13:06 ` erik quanstrom 2013-06-05 13:13 ` Don Bailey 0 siblings, 2 replies; 9+ messages in thread From: yaroslav @ 2013-06-05 11:38 UTC (permalink / raw) To: 9fans in /sys/src/cmd/lp/lpdaemon.c:297,310 These info.host[strlen(info.host)] = '\0'; … info.user[strlen(info.user)] = '\0'; look nonsence as zeros are placed exactly where they already are. Should read as in following instead: info.host[NAMELEN] = '\0'; … info.user[NAMELEN] = '\0'; shoudn't it? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 11:38 [9fans] lpdaemon yaroslav @ 2013-06-05 13:06 ` erik quanstrom 2013-06-05 13:13 ` Don Bailey 1 sibling, 0 replies; 9+ messages in thread From: erik quanstrom @ 2013-06-05 13:06 UTC (permalink / raw) To: 9fans i agree ... applied to 9atom. Subject: [sources] applied patch: /n/atom/patch/applied/lpdaemonnit Reply-To: sources@9atom.org email quanstro@quanstro.net readme >From: yaroslav <yarikos@gmail.com> >Subject: [9fans] lpdaemon in /sys/src/cmd/lp/lpdaemon.c:297,310 These info.host[strlen(info.host)] = '\0'; … info.user[strlen(info.user)] = '\0'; look nonsence as zeros are placed exactly where they already are. Should read as in following instead: info.host[NAMELEN] = '\0'; … info.user[NAMELEN] = '\0'; removed files /sys/src/cmd/lp/lpdaemon.c lpdaemon.c /sys/src/cmd/lp/lpdaemon.c lpdaemon.c lpdaemon.c.orig:299,305 - lpdaemon.c:299,305 strncpy(info.host, "unknown", NAMELEN); else strncpy(info.host, (const char *)&ap[1], NAMELEN); - info.host[strlen(info.host)] = '\0'; + info.host[NAMELEN] = '\0'; break; case 'P': if (ap[1] == '\0') lpdaemon.c.orig:306,312 - lpdaemon.c:306,312 strncpy(info.user, "unknown", NAMELEN); else strncpy(info.user, (const char *)&ap[1], NAMELEN); - info.user[strlen(info.user)] = '\0'; + info.user[NAMELEN] = '\0'; break; } } ------ merge...backup...copy... cpfile lpdaemon.c /n/dist/sys/src/cmd/lp/lpdaemon.c # remove these files if you want. I will not remove them for you # ($patch/undo will not restore them) done ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 11:38 [9fans] lpdaemon yaroslav 2013-06-05 13:06 ` erik quanstrom @ 2013-06-05 13:13 ` Don Bailey 2013-06-05 13:20 ` erik quanstrom 2013-06-05 13:38 ` Friedrich Psiorz 1 sibling, 2 replies; 9+ messages in thread From: Don Bailey @ 2013-06-05 13:13 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs The first opportunity to write a nil byte should always be taken. Using sizeof only means that in corner cases memory disclosure may occur between where the nil should be and the end of the array. While this isn't a security critical app, it is still good coding practice. x = strlen(info.host) < sizeof info.host ? strlen() : sizeof ; info.host[x] = 0; D On Jun 5, 2013, at 5:38 AM, yaroslav <yarikos@gmail.com> wrote: > in /sys/src/cmd/lp/lpdaemon.c:297,310 > > These > info.host[strlen(info.host)] = '\0'; > … > info.user[strlen(info.user)] = '\0'; > > look nonsence as zeros are placed exactly where they already are. > Should read as in following instead: > > info.host[NAMELEN] = '\0'; > … > info.user[NAMELEN] = '\0'; > > shoudn't it? > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 13:13 ` Don Bailey @ 2013-06-05 13:20 ` erik quanstrom 2013-06-05 13:40 ` Don Bailey 2013-06-05 13:38 ` Friedrich Psiorz 1 sibling, 1 reply; 9+ messages in thread From: erik quanstrom @ 2013-06-05 13:20 UTC (permalink / raw) To: 9fans On Wed Jun 5 09:15:11 EDT 2013, don.bailey@gmail.com wrote: > The first opportunity to write a nil byte should always be taken. > Using sizeof only means that in corner cases memory disclosure may > occur between where the nil should be and the end of the array. While > this isn't a security critical app, it is still good coding practice. > > x = strlen(info.host) < sizeof info.host ? strlen() : sizeof ; > info.host[x] = 0; let's start at the beginning. strncpy is not good coding practice. and lpdaemon is not well written by today's standards. ☺ however, unless i'm missing something, the code has exactly that. /sys/src/cmd/lp/lpdaemon.c:297,310 case 'H': if (ap[1] == '\0') strncpy(info.host, "unknown", NAMELEN); else strncpy(info.host, (const char *)&ap[1], NAMELEN); info.host[NAMELEN] = '\0'; break; case 'P': if (ap[1] == '\0') strncpy(info.user, "unknown", NAMELEN); else strncpy(info.user, (const char *)&ap[1], NAMELEN); info.user[NAMELEN] = '\0'; break; - erik ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 13:20 ` erik quanstrom @ 2013-06-05 13:40 ` Don Bailey 0 siblings, 0 replies; 9+ messages in thread From: Don Bailey @ 2013-06-05 13:40 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs Not exactly. But, functionally close enough. I skipped commenting on strncpy to ignore the plethora of issues with lpd and focus on the question at hand. D On Jun 5, 2013, at 7:20 AM, erik quanstrom <quanstro@quanstro.net> wrote: > On Wed Jun 5 09:15:11 EDT 2013, don.bailey@gmail.com wrote: >> The first opportunity to write a nil byte should always be taken. >> Using sizeof only means that in corner cases memory disclosure may >> occur between where the nil should be and the end of the array. While >> this isn't a security critical app, it is still good coding practice. >> >> x = strlen(info.host) < sizeof info.host ? strlen() : sizeof ; >> info.host[x] = 0; > > let's start at the beginning. strncpy is not good coding practice. > and lpdaemon is not well written by today's standards. ☺ > > however, unless i'm missing something, the code has exactly that. > > /sys/src/cmd/lp/lpdaemon.c:297,310 > case 'H': > if (ap[1] == '\0') > strncpy(info.host, "unknown", NAMELEN); > else > strncpy(info.host, (const char *)&ap[1], NAMELEN); > info.host[NAMELEN] = '\0'; > break; > case 'P': > if (ap[1] == '\0') > strncpy(info.user, "unknown", NAMELEN); > else > strncpy(info.user, (const char *)&ap[1], NAMELEN); > info.user[NAMELEN] = '\0'; > break; > > - erik > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 13:13 ` Don Bailey 2013-06-05 13:20 ` erik quanstrom @ 2013-06-05 13:38 ` Friedrich Psiorz 2013-06-05 13:54 ` Don Bailey 1 sibling, 1 reply; 9+ messages in thread From: Friedrich Psiorz @ 2013-06-05 13:38 UTC (permalink / raw) To: 9fans I think your code is wrong. If the NUL byte is present, it doesn't do anything, however if it is not there, strlen will read more than it should, and possibly try to read some invalid address. In case info.host is a fixe size array, a simple info.host[sizeof info.host - 1] = 0; would do. Am 05.06.2013 15:13, schrieb Don Bailey: > The first opportunity to write a nil byte should always be taken. Using sizeof only means that in corner cases memory disclosure may occur between where the nil should be and the end of the array. While this isn't a security critical app, it is still good coding practice. > > x = strlen(info.host) < sizeof info.host ? strlen() : sizeof ; > info.host[x] = 0; > > D > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 13:38 ` Friedrich Psiorz @ 2013-06-05 13:54 ` Don Bailey 2013-06-05 14:09 ` erik quanstrom 0 siblings, 1 reply; 9+ messages in thread From: Don Bailey @ 2013-06-05 13:54 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs You're absolutely correct if the length of value to be copied is not validated prior to the copy. Then, an invalid page could be hit if no nil is present within the array or beyond. I wasn't providing a verbatim patch (notice the function and operator weren't filled in). I was just providing the sequence of events that should occur. Eric points out correctly that strncpy effectively performs the first operation on the user's behalf. The second is achieved through the write to N. To be verbose, my bypassing of strncpy is due to issues I've encountered in multi-threaded code. e.g. Don't trust libc copy functions in MT envs, always check post call. An interesting and sometimes desirable effect of crashing on an invalid page read is that if memory can be corrupted, a consistent unexploitable crash is better than entering a context where the bug becomes exploitable. D On Jun 5, 2013, at 7:38 AM, Friedrich Psiorz <f.psiorz@gmx.de> wrote: > I think your code is wrong. If the NUL byte is present, it doesn't do > anything, however if it is not there, strlen will read more than it > should, and possibly try to read some invalid address. > In case info.host is a fixe size array, a simple > info.host[sizeof info.host - 1] = 0; > would do. > > Am 05.06.2013 15:13, schrieb Don Bailey: >> The first opportunity to write a nil byte should always be taken. Using sizeof only means that in corner cases memory disclosure may occur between where the nil should be and the end of the array. While this isn't a security critical app, it is still good coding practice. >> >> x = strlen(info.host) < sizeof info.host ? strlen() : sizeof ; >> info.host[x] = 0; >> >> D > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 13:54 ` Don Bailey @ 2013-06-05 14:09 ` erik quanstrom 2013-06-05 14:29 ` Don Bailey 0 siblings, 1 reply; 9+ messages in thread From: erik quanstrom @ 2013-06-05 14:09 UTC (permalink / raw) To: 9fans > You're absolutely correct if the length of value to be copied is not > validated prior to the copy. Then, an invalid page could be hit if no > nil is present within the array or beyond. wrong. strncpy only copies up to the specified maximum. the code is ugly but correct. > To be verbose, my bypassing of strncpy is due to issues I've > encountered in multi-threaded code. e.g. Don't trust libc copy > functions in MT envs, always check post call. this sounds like your saying that because you had trouble in a multithreaded unix application, then without examining the code at hand, it is pronounced to have the same issue. that sounds like equivocation to me. the code is correct. and in all cases nul-terminated, and any unused bytes are 0. i only object to strncpy because it requires extra work. seprint, snprint are a bit heavy weight but tend to produce cleaner looking code. ymmv. - erik ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [9fans] lpdaemon 2013-06-05 14:09 ` erik quanstrom @ 2013-06-05 14:29 ` Don Bailey 0 siblings, 0 replies; 9+ messages in thread From: Don Bailey @ 2013-06-05 14:29 UTC (permalink / raw) To: Fans of the OS Plan 9 from Bell Labs You get that I'm talking about the subsequent read back after copy, right? No need to be so competitive :) Also, you're making strange presumptions about me having presumptions. I'm not trying to say you're wrong or a poor coder, Erik. I was simply offering my point of view. Before this thread dissolves into typical Plan 9 waste I'll admit that my first mistake was to make the poor decision to reply to a 9fans email. D On Jun 5, 2013, at 8:09 AM, erik quanstrom <quanstro@quanstro.net> wrote: >> You're absolutely correct if the length of value to be copied is not >> validated prior to the copy. Then, an invalid page could be hit if no >> nil is present within the array or beyond. > > wrong. strncpy only copies up to the specified maximum. > the code is ugly but correct. > >> To be verbose, my bypassing of strncpy is due to issues I've >> encountered in multi-threaded code. e.g. Don't trust libc copy >> functions in MT envs, always check post call. > > this sounds like your saying that because you had trouble in > a multithreaded unix application, then without examining the > code at hand, it is pronounced to have the same issue. > > that sounds like equivocation to me. the code is correct. > and in all cases nul-terminated, and any unused bytes are > 0. > > i only object to strncpy because it requires extra work. seprint, > snprint are a bit heavy weight but tend to produce cleaner looking > code. ymmv. > > - erik > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-05 14:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-05 11:38 [9fans] lpdaemon yaroslav 2013-06-05 13:06 ` erik quanstrom 2013-06-05 13:13 ` Don Bailey 2013-06-05 13:20 ` erik quanstrom 2013-06-05 13:40 ` Don Bailey 2013-06-05 13:38 ` Friedrich Psiorz 2013-06-05 13:54 ` Don Bailey 2013-06-05 14:09 ` erik quanstrom 2013-06-05 14:29 ` Don Bailey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).