fsck_msdos: out of boundary access

2014-06-08 Thread Tobias Stoeckmann
Hi, Google's Android team uses a modified fsck_msdos version from FreeBSD, fixing issues in their local repositories. No license adjustments, so we are free to merge them into ours. One of these fixes covers an out of boundary access that can occur if filesystem points to a cluster outside of

fsck_msdos: off by one for FAT12

2014-06-09 Thread Tobias Stoeckmann
Hi, our guard pages just told me that writefat is vulnerable to an off by one for FAT12 filesystems. They shouldn't be that common anymore, but better be safe than sorry. The diff looks confusing at first, but this default-block is just split into two cluster blocks, as it's done in readfat

Re: fsck_msdos: off by one for FAT12

2014-06-09 Thread Tobias Stoeckmann
On Mon, Jun 09, 2014 at 09:16:42PM +0200, Tobias Stoeckmann wrote: + cl++; [...] + *p |= (u_char)(fat[cl + 1].next 4); + *p++ = (u_char)(fat[cl + 1].next 4); And here the correct diff, cl + 1 must not be done after cl++ ... Index

howmany macro: integer overflow

2014-06-14 Thread Tobias Stoeckmann
Hi, the howmany macro as used in param.h and select.h is prone to an integer overflow. It adds divisor-1 to the base value, which means that it COULD overflow. Most of the times, the howmany macro is used with file descriptors and polling, would be hard to overflow it. Especially due to C

fsck_msdos: fix possible infinite loop

2014-06-14 Thread Tobias Stoeckmann
Hi, fsck_msdos is prone to an infinite loop if cluster chains in the filesystem are infinite: FAT handles clusters as linked lists, and at worst this list can be cyclic. Android included a fix from Samsung for this issue, with the commit b6ee08aadb580341a4d80943741b80de16a88b5d:

Re: fsck_msdos: fix possible infinite loop

2014-06-14 Thread Tobias Stoeckmann
On Sat, Jun 14, 2014 at 11:25:22AM -0400, Kenneth Westerback wrote: /* follow the chain to its end (hopefully) */ - for (p = head; + for (len = fat[head].length, p = head; (n = fat[p].next) = CLUST_FIRST n

valid FAT32 for newfs and fsck

2014-06-16 Thread Tobias Stoeckmann
Hi, ran into this problem that NetBSD fixed a few months ago, which means that this diff is a NetBSD merge: Our newfs_msdos creates an invalid FAT32 file system. The first free cluster of FSinfo points to cluster #2, which is used by the root directory. Next to that, fsck_msdos is a bit too

sys/msdosfs: off by one

2014-06-16 Thread Tobias Stoeckmann
Hi, there is a potential off by one in function fillinusemap() leading to possible out of boundary access (32 bytes after allocated area). pmp-pm_inusemap is allocated in msdosfs_vfsops.c like this: bmapsiz = (pmp-pm_maxcluster + N_INUSEBITS - 1) / N_INUSEBITS; pmp-pm_inusemap =

Re: sys/msdosfs: off by one

2014-06-17 Thread Tobias Stoeckmann
On Mon, Jun 16, 2014 at 04:43:02PM -0700, John-Mark Gurney wrote: FreeBSD fixed this by increasing the malloc size: https://svnweb.freebsd.org/changeset/base/r126086 Which is actually the correct way to do here! pmp-pm_maxcluster is the largest valid _index_ of pmp-pm_inusemap, therefore we

fsck_msdos: out of boundary on file truncation

2014-06-17 Thread Tobias Stoeckmann
Hi, fsck_msdos checks for linked cluster chains, which means that two chains cross each other at the same cluster. If 1 links to 3 and 2 links to 3, the cluster chains starting at 1 and 2 are linked. This error condition can be fixed during phase 2, either one or both chains are dropped or the

Re: howmany macro: integer overflow

2014-06-18 Thread Tobias Stoeckmann
On Wed, Jun 18, 2014 at 08:20:16AM +0200, Alexander Hall wrote: RCS file: /cvs/src/sys/sys/param.h,v Where else could howmany come from? Can you be sure it's safe to use here? There are other places where howmany is defined, I can prepare a diff that will cover all definitions. For now, I

fsck_msdos: long filename out of boundary access

2014-06-18 Thread Tobias Stoeckmann
Hi, while constructing the long file name of a directory entry, it is possible to access the longName array out of bounds. Long file names in FAT are supported by additional long filename directory entries. They are kept in reverse (from top to bottom) with an additional 0x40 flag at the start,

Re: exclude-list in mtree (2)

2014-06-20 Thread Tobias Stoeckmann
On Fri, Jun 20, 2014 at 04:34:19PM +0200, Manuel Giraud wrote: + lbuf = NULL; + while ((buf = fgetln(fp, len))) { + if (buf[len - 1] == '\n') { + if (len == 1) + continue; + buf[len - 1] = '\0'; +

fsck_msdos: validate basic information

2014-06-23 Thread Tobias Stoeckmann
Hi, there should be some more validations in boot.c. Not just to validate the filesystem, but to make sure that the program won't crash later on because it assumes some values... First chunk: The sectors per cluster have to be a power of 2, otherwise it's possible to send fsck_msdos over the

Re: sys/msdosfs: possible kernel crash

2014-06-23 Thread Tobias Stoeckmann
And again... Could have saved myself time by checking FreeBSD in detail first. They fixed the issue by adding a simple check: According to FAT specs, the size of a cluster shall not exceed 64 KB. That is even a rare case, 32 KB shouldn't be crossed for compatibility reasons. Therefore, it's

newfs_msdos: proper boot signature

2014-06-24 Thread Tobias Stoeckmann
Hi, time to merge another fix from NetBSD (and FreeBSD who applied it too): If sector size is not 512, the boot signature is placed at a wrong position. It always has to be at offset 510/511, not sector size - 2. # dd if=/dev/zero of=fat.iso bs=1M count=1 # vnconfig vnd0c fat.iso # newfs_msdos

boot: off-by-one

2014-06-26 Thread Tobias Stoeckmann
Hi, in boot, we have an off-by-one error in readline. When the user ends input with enter, the string will be ended twice, like: p[1] = *p = '\0'; Comparing readline with read_conf (reading commands from file), there is no need to double end the input line. Tobias Index: cmd.c

boot: overflow in bootarg on truncation

2014-06-26 Thread Tobias Stoeckmann
Hi, this is of interest for amd64 and i386 only: The bootarg code is a code collection to manage a bootarg_list, which can be assembled into contiguous code with makebootargs(). makebootargs takes two arguments: pointer and available space. Upon return, the available space variable will be

boot/zboot: cmd.c merge

2014-06-29 Thread Tobias Stoeckmann
Hi, this diff merges cmd.c files of sys/stand/boot and sys/arch/zaurus/stand/zboot. Back in time, that file was copied to add clear command for zaurus. Revision 1.2 of zaurus' file added some clean ups which could be merged back. Otherwise, get the changes of stand/boot into stand/zboot, too.

Re: boot/zboot: cmd.c merge

2014-06-30 Thread Tobias Stoeckmann
On Sun, Jun 29, 2014 at 08:40:53PM +0200, Tobias Stoeckmann wrote: Greatly reduces diff-Output between these files: Just clear command, the same way it was back then. After feedback from Theo, just kill clear command and therefore use cmd.c from stand/boot in zboot. Any zaurus user around

fsck_msdos: memleak merge with NetBSD

2014-06-30 Thread Tobias Stoeckmann
Hi, this diff merges NetBSD's revision 1.20 into our tree: There are some memory leaks in resetDosDirSection. This is not a simple merge, I have added some things: - rootDir was not properly freed in NetBSD's commit (actually it's put into a free dir entry queue) - also free memory if root

Re: boot/zboot: cmd.c merge

2014-07-04 Thread Tobias Stoeckmann
On Sun, Jun 29, 2014 at 08:40:53PM +0200, Tobias Stoeckmann wrote: cc -c works for zaurus' cmd.c. I don't have a zaurus, so it would be nice if a zaurus owner can test these changes. Got feedback from zaurus users. The Makefile was missing another change: It still listed cmd.c in SRCS

Re: fsck_msdos: memleak merge with NetBSD

2014-07-08 Thread Tobias Stoeckmann
On Tue, Jul 08, 2014 at 03:01:58PM -0400, Ted Unangst wrote: I think it's safe to just have one goto label? free(NULL) is fine. That's true. If we go into that direction, we can also use the cleanup function that would be done at the end of directory processing. Also put two variables static

syslogd: patch for CVE-2014-3634

2014-10-12 Thread Tobias Stoeckmann
Hi, our syslogd is also vulnerable to rsyslog's CVE-2014-3634. The CVE is about parsing the priority from network clients. The priority boundary isn't properly checked, which could lead to out of bounds access later on. sysklogd's commit message is pretty extensive, so have a read here:

Re: syslogd: patch for CVE-2014-3634

2014-10-12 Thread Tobias Stoeckmann
On Sun, Oct 12, 2014 at 11:47:36AM -0700, Philip Guenther wrote: Have you actually managed to make it crash? I've already committed a check for this when this first came out, mapping out of bounds pri values to LOG_USER, and at that time no one was able to crash the code without the check...

patch: crash on large files

2014-11-09 Thread Tobias Stoeckmann
Hi, our patch implementation supports lines with a maximum of 8192 chars, which should be reasonably large enough. If files cannot be patched in memory, they are written into temporary files -- also known as plan b. For plan b, the maximum line length is 1024, which is still more than enough.

boot: readline off by one

2014-01-11 Thread Tobias Stoeckmann
Hi, in boot, we have an off-by-one error in readline. When the user ends input with enter, the string will be ended twice, like: p[1] = *p = '\0'; Therefore we have to make sure that two bytes are still free, not just one. Not sure why it has to be handled like this, but the fix is easy

Re: boot: readline off by one

2014-01-11 Thread Tobias Stoeckmann
On Sat, Jan 11, 2014 at 12:43:33PM +0100, Tobias Stoeckmann wrote: p[1] = *p = '\0'; If this is actually intended behaviour, it's off from read_conf, which ends cmd_buf with just one terminating \0 character.

getusershell: off by one

2014-01-16 Thread Tobias Stoeckmann
Hi, the library function getusershell(3) assumes that the smallest possible line in /etc/shells would be 3 chars (slash, a char, new line): In that case, there are at max sb.st_size / 3 of lines. Well, that is not entirely correct. The last line could be just 2 chars, skipping the trailing new

lpd: race condition

2014-01-17 Thread Tobias Stoeckmann
Hi, lpd wants to verify that it doesn't open a symbolic link, checking with lstat(), then open()ing the file. The only reason I can see that the code does not simply use O_NOFOLLOW is a different return value if it encounters a symlink (maybe I am wrong here, would like to get feedback on this

libedit: retain local changes

2014-01-18 Thread Tobias Stoeckmann
Hi, back in OpenBSD 5.0 times, there was a merge with NetBSD's libedit. It was supposed to retain local modifications, but unfortunately they got lost. In el.c, they got reintroduced with revision 1.18, but the following ones are still lost: history.c: revision 1.13 term.c: revision 1.13

Re: lpd: race condition

2014-01-19 Thread Tobias Stoeckmann
On Mon, Jan 20, 2014 at 10:11:53AM +1300, Philip Guenther wrote: On Sun, Jan 19, 2014 at 10:48 AM, Todd C. Miller todd.mil...@courtesan.com wrote: Perhaps something like this? Only compile-tested. Looks good. We also need to fix the 'S' line parsing code in sendit() and printit() in

Re: bug fix: 'opencvs log' mangles RCS branch number

2012-07-01 Thread Tobias Stoeckmann
On Sat, Jun 30, 2012 at 09:03:05PM -0400, Ted Unangst wrote: Comparing cvs log output between GNU CVS 1.11.1p1 and OpenCVS... - mwb: 1.1.1 + mwb: 1.1.0.1 OpenCVS mangles the revision number. So it looks like branch numbers that we read in don't need to be translated

race condition in gzsig

2013-03-09 Thread Tobias Stoeckmann
Hi, there is a race condition in gzsig. It needs some fancy vector to step in, but better safe than sorry (especially if the fix is pretty cheap). The race happens between opening the input file and applying the access rights to the temporary one. If an attacker is able to replace the input

gzsig: avoid endless loop on input error

2013-03-09 Thread Tobias Stoeckmann
Hi, while skipping some header sections of a *.gz file, gzsig uses the function getc without taking a possible EOF return value into account. On error, the tool should obviously stop with an error message, therefore leaving sign() or verify() with -1. Tobias Index: sign.c

sdiff: buffer overflow in diffargv

2013-03-30 Thread Tobias Stoeckmann
Hi, sdiff makes a false assumption about diffargv: /* * Allocate memory for diff arguments and NULL. * Each flag has at most one argument, so doubling argc gives an * upper limit of how many diff args can be passed. argv[0], * file1, and file2 won't have arguments so doubling them will *

sdiff: memory leak in parsecmd

2013-03-30 Thread Tobias Stoeckmann
Hi, there is a memory leak in sdiff occurring while parsing ed commands in parsecmd (which is feeded basically by diff's output through a pipe). The function xfgets uses fparseln, which means that the return value should be freed. This is not the case for the variable line. Also, there is a

pflogd: move_log cleanup

2013-03-30 Thread Tobias Stoeckmann
Hi, the privileged part of pflogd is responsible to move bad log files out of the way, i.e. ones that cannot be parsed. The current code could end up in an endless loop if all possible files exist. Also, it doesn't clean up after a failed rename() attempt, leaving a dead file behind. I have

Active PS/2 Multiplexing

2013-04-01 Thread Tobias Stoeckmann
Hi, this issue is already quite old, but let's see if anyone else has still issues with active PS/2 multiplexing. Active PS/2 multiplexing is a method to attach multiple input devices on your AUX PS/2 slot. This should be as of today of no real use anymore, since it's easier to just add a USB

Re: Active PS/2 Multiplexing

2013-04-15 Thread Tobias Stoeckmann
Hello mike, and hello list, this is the first feedback I have received as of now, and nice to hear that the diff helps someone else, too. Having no negative feedback, I would say it didn't hurt anyone out there (yet). So if there are no further objections or feedback, I would go for the diff

OpenCVS - new RCS parser

2010-10-01 Thread Tobias Stoeckmann
with current OpenCVS; it works fine for us. Please don't hesitate to give me feedback, as a user or as a developer. Tobias Stoeckmann Index: Makefile === RCS file: /home/tobias/anoncvs/src/usr.bin/cvs/Makefile,v retrieving revision

Re: OpenCVS - new RCS parser

2010-10-07 Thread Tobias Stoeckmann
a supplied RCS file fully with current and new parser and compares the data structures in memory. If you see ANY output, PLEASE send me the RCS file! Usage: opencvs /path/to/RCS/file,v http://www.stoeckmann.org/cvs.tar.gz With kind regards, Tobias Stoeckmann

Re: patch: crash on large files

2014-11-14 Thread Tobias Stoeckmann
Anyone into reviewing? Or suggestions? In short, this diff verifies that we don't try to write lines into the file buffer which are longer than the longest one allowed. Tobias Index: inp.c === RCS file:

patch: check patch file size

2014-11-16 Thread Tobias Stoeckmann
Hi, p_filesize is of type long, but we assign an off_t. Before assignment, check if it will fit. Also, check if fstat was successful or not. Tobias Index: pch.c === RCS file: /cvs/src/usr.bin/patch/pch.c,v retrieving revision

Re: patch: check patch file size

2014-11-16 Thread Tobias Stoeckmann
On Sun, Nov 16, 2014 at 09:39:32PM +0100, Alexander Bluhm wrote: Can we change p_filesize type to off_t instead? Definitely, but it takes closer looks for a review. Diff exists, I was just not sure if it's worth to support patch files which are larger than 2 GB on 32 bit systems. Tobias

Re: less strlen in bgpctl

2014-11-17 Thread Tobias Stoeckmann
On Sun, Nov 16, 2014 at 06:38:24PM -0500, Ted Unangst wrote: Not sure how I ended up reading this file, but all the redundant strlen calls make me twitchy. I agree, but have you considered the other parser.c files with match_token and same use of strlen(word), too?

ospf6ctl: memleak in parser

2014-11-17 Thread Tobias Stoeckmann
Hi, after using the temporary buffer ps for parse_addr, it is not released after successful operation. Tobias Index: parser.c === RCS file: /cvs/src/usr.sbin/ospf6ctl/parser.c,v retrieving revision 1.12 diff -u -p -r1.12 parser.c

patch: always validate hunk size

2014-11-17 Thread Tobias Stoeckmann
Hi, the hunk size is basically limitated to MAXHUNKSIZE, although the implementation consideres it as a soft limit: it's possible to get one step above this limit. It's set to 100,000 but hunkmax can be legally set to 128,000... This diff unifies the grow check and avoids calling realloc()

patch: properly check NULL return values

2014-11-17 Thread Tobias Stoeckmann
Hi, savestr is a function very similar to strdup. If it runs into an out of memory condition, its behavior depends on the current status of the program. If patch is in plan a mode, it will set an out_of_memory flag, returns NULL and patch will try later on with plan b again. If it is in plan b,

patch: avoid reading after end of string

2014-11-18 Thread Tobias Stoeckmann
Hi, on a diff with a missing new line, it is possible that patch will read past the terminating NUL character. Tobias Index: pch.c === RCS file: /cvs/src/usr.bin/patch/pch.c,v retrieving revision 1.42 diff -u -p -r1.42 pch.c ---

Re: patch: avoid reading after end of string

2014-11-18 Thread Tobias Stoeckmann
On Tue, Nov 18, 2014 at 04:32:17PM +0100, Otto Moerbeek wrote: I seem to remember other code guarantees that lines are always termined by '\n'. Specifically plan_a(), plan_b() and ifetch(); Not in this case. We use the output of fgets. This would be a valid contextual diff: $ echo a a $

patch: introduce strtolinenum (instead of atol)

2014-11-18 Thread Tobias Stoeckmann
Hi, the numbers in patch files are parsed with atol(), which is not properly checked for all cases: - atol() accepts leading spaces, whereas the surrounding code will iterate through digits to skip that number... If there is a plus or minus sign in front, this for-loop would fail, too. -

patch: tedu sccs support

2014-11-19 Thread Tobias Stoeckmann
Hi, as we don't even have the get command in base, there is no need to support SCCS files anymore. I also remember that we removed the SCCS IDs from source files so... Time to tedu sccs? Tobias Index: common.h === RCS file:

Re: patch: tedu sccs support

2014-11-20 Thread Tobias Stoeckmann
On Wed, Nov 19, 2014 at 10:02:33PM -0500, Ted Unangst wrote: Are we still tracking upstream patch in any way? I think we've diverged in a few places, but there still is an upstream, yes? I don't think so. Looking at the past, the first version was written by Larry Wall who licensed it to FSF

rcs: uninitialized pointer leads to segfault

2014-11-22 Thread Tobias Stoeckmann
Hi, as jsg@ pointed out, rcs will segfault reliably when using malloc.conf with 'J' (the pointer in question is filled with d0's). The pointer rp_delta is checked at the end of rcsparse_delta. If it's non-NULL, it will be included into a linked list; line 1181 of rcsparse.c. This RCS file

Re: rcs: uninitialized pointer leads to segfault

2014-11-22 Thread Tobias Stoeckmann
On Sat, Nov 22, 2014 at 11:45:02AM +0100, Tobias Stoeckmann wrote: as jsg@ pointed out, rcs will segfault reliably when using malloc.conf with 'J' (the pointer in question is filled with d0's). As Theo suggested, xcalloc will take care of this pointer and other struct entries which

Re: locate(1): ignore paths longer than MAXPATHLEN

2014-11-23 Thread Tobias Stoeckmann
On Sun, Nov 23, 2014 at 06:59:59PM +0100, Nicolas Bedos wrote: Index: src/usr.bin/locate//code/locate.code.c === RCS file: /cvs/src/usr.bin/locate/code/locate.code.c,v retrieving revision 1.17 diff -u -p -u -r1.17 locate.code.c

patch: fix segfault on revision + empty file

2014-11-24 Thread Tobias Stoeckmann
Hi, patch will fail with a segmentation fault in plan a if it encounters a diff with a revision (Prereq line) when the input file is empty. i_womp will be set to NULL to avoid mmapping 0 bytes, but later on it will be scanned for the supplied revision. The fix is simple: avoid scanning i_womp

paste: release file descriptors earlier

2014-11-24 Thread Tobias Stoeckmann
Hi, the function parallel() should release file descriptors just like sequential() does: If we reach EOF, close it -- except we were reading stdin. Tobias Index: paste.c === RCS file: /cvs/src/usr.bin/paste/paste.c,v retrieving

Re: locate(1): ignore paths longer than MAXPATHLEN

2014-11-24 Thread Tobias Stoeckmann
On Mon, Nov 24, 2014 at 08:37:47PM +0100, Nicolas Bedos wrote: In locate.code.c 'mbuf' is never free()d: it is only allocated for the last line of input and after processing this line the program ends. I hope it is ok. I would free() it nontheless outside the while loop. For the sake of

patch: integer overflows and oob memory access

2014-11-25 Thread Tobias Stoeckmann
Hi, it is possible to overflow line numbers in patch; this diff cares about the lines specified in diff files. If such an overflow happens with unified diffs, out of bound memory access can occur. If you have a 32 bit system, take this one (LONG_MAX = 2^31 - 1): --- a Sat Nov 15 00:25:29 2014

Re: patch: properly check NULL return values

2014-11-26 Thread Tobias Stoeckmann
Hi, turns out that there are a few more bad savestr calls even inside of pch.c. Some of the code pathes are quite obvious that a NULL return value would lead to a null pointer dereference, others would last longer before dereferencing. Carefully reviewing the savestr calls, the rule of thumb

Re: locate(1): ignore paths longer than MAXPATHLEN

2014-11-26 Thread Tobias Stoeckmann
On Wed, Nov 26, 2014 at 09:48:15PM +0100, Nicolas Bedos wrote: Last update, with Tobias's help. The following diff - changes MAXPATHLEN from sys/param.h to PATH_MAX from limits.h - adds a missing prototype for sane_count - locate.bigram and locate.code now abort when reading a pathname

syslogd: properly validate config

2014-11-27 Thread Tobias Stoeckmann
Hi, the facility number is not properly validated while parsing the configuration file -- it is possible to supply a number which is larger than LOG_NFACILITIES, therefore accessing memory outside of f_pmask's boundaries. # echo 10.debug;syslog,user.info /var/log/messages my.conf # syslogd

Re: syslogd: properly validate config

2014-11-27 Thread Tobias Stoeckmann
On Thu, Nov 27, 2014 at 01:29:48PM -0700, Todd C. Miller wrote: I think it would be better for decode() to just return -1 in this case. The validation looks a bit like a magic number there, but this could prevent issues of other decode()-users, too... So yeah, I think that is worth it: Index:

patch: Support long lines in Plan B

2014-11-29 Thread Tobias Stoeckmann
Hi, this diff doesn't just fix the division by zero for input files with lines longer than 1023 chars in Plan B mode, it actually removes this line limit! Before: $ dd if=/dev/zero bs=1 count=1024 | tr '\0' a a $ dd if=/dev/zero bs=1 count=1024 | tr '\0' b b $ diff -u a b a.diff $ patch -x 8

Re: patch: Support long lines in Plan B

2014-12-07 Thread Tobias Stoeckmann
Anyone? On Sat, Nov 29, 2014 at 05:40:31PM +0100, Tobias Stoeckmann wrote: Hi, this diff doesn't just fix the division by zero for input files with lines longer than 1023 chars in Plan B mode, it actually removes this line limit! Before: $ dd if=/dev/zero bs=1 count=1024 | tr '\0

Re: skgpio crash on i386 (#604)

2014-12-11 Thread Tobias Stoeckmann
On Thu, Dec 11, 2014 at 08:21:12PM +, Miod Vallat wrote: Hi, I'm encountering system crash during boot with latest snapshot. Turns out that it works fine when I disable skgpio through UKC. Try this. Okay tobias@, too. ;) The system boots nicely again, thanks!

chmod: range checks

2014-12-12 Thread Tobias Stoeckmann
Hi, chmod doesn't check if the program name is at least 3 characters long before checking its index 2. Also, there is a compiler warning about signed vs unsigned when val is used. In one instance, it's used with strtoul, in another with strtol, checking its ranges. It's okay due to automatic

Re: chmod: range checks

2014-12-12 Thread Tobias Stoeckmann
On Fri, Dec 12, 2014 at 10:42:21AM -0800, patrick keshishian wrote: Just throwing this out there: will this program ever get installed with filename shorter than ch{grp,mod,own,flags}? No. It's still a form of input validation. Therefore, it should be done. And a user can create such a link

patch: fix arbitrary ed command allowance

2014-12-13 Thread Tobias Stoeckmann
Hi, patch accepts arbitrary ed commands after encountering s. The s ed command does not expect any further input, which makes it a one line command like d. Yet, patch sends any lines until . unchecked to ed through its pipe, allowing command execution. Example: $ ls ed.diff $ cat ed.diff 0a

Re: patch: fix arbitrary ed command allowance

2014-12-13 Thread Tobias Stoeckmann
On Sat, Dec 13, 2014 at 10:57:42AM -0500, Daniel Dickman wrote: - (*t == 'a' || *t == 'c' || *t == 'd' || *t == 'i' || *t == 's')) { + strchr(acdis, *t) != NULL) { doesn't this change the semantics slightly? i haven't looked at the context beyond

patch: safer temp file handling

2014-12-13 Thread Tobias Stoeckmann
=== RCS file: temp.c diff -N temp.c --- /dev/null 1 Jan 1970 00:00:00 - +++ temp.c 13 Dec 2014 19:38:20 - @@ -0,0 +1,110 @@ +/* $OpenBSD$ */ +/* + * Copyright (c) 2014 Tobias Stoeckmann tob...@openbsd.org

compress: funopen error handling

2015-01-31 Thread Tobias Stoeckmann
Hi, this diff adds error handling for funopen() failure. I have also fixed a whitespace issue for proper intendation. Tobias Index: usr.bin/compress/zopen.c === RCS file: /cvs/src/usr.bin/compress/zopen.c,v retrieving revision

libssl: signal races in capability checks

2015-03-14 Thread Tobias Stoeckmann
Hi, grep'ed the tree for siglongjmp calls, and spotted possible offenders in libssl's code. The code in question checks hardware capabilities for ARM, S390x, and SPARCv9. The code will call some routines that could trigger SIGILL (or SIGBUS), which is caught with an own signal handler. This

Re: tail: -r mem leak with non-regular files

2015-03-27 Thread Tobias Stoeckmann
On Thu, Mar 26, 2015 at 11:41:23PM +0100, Tobias Stoeckmann wrote: The less obvious one is in an error path. As tl-l is always of fixed size (BSZ), we can just change the struct to have a BSZ sized array in it. This removes the need to do checks in the error path completely. While

sort: useless use of cat

2015-03-31 Thread Tobias Stoeckmann
I see a useless use of cat in here. Manual page: When called with the -d option, it must decompress standard input to standard output. Index: file.c === RCS file: /cvs/src/usr.bin/sort/file.c,v retrieving revision 1.4 diff -u -p -u

Re: sort output file permissions

2015-03-31 Thread Tobias Stoeckmann
Setting permissions without setting owner and group can have rather inconvenient results. See this example with latest code: $ touch test $ chgrp wheel test $ chmod g+w test $ ls -l test -rw-rw-r-- 1 tobias wheel 0 Mar 31 20:05 test $ ./sort -o test test $ ls -l test -rw-rw-r-- 1 tobias

patch: safer temp file handling

2015-03-29 Thread Tobias Stoeckmann
$ */ +/* + * Copyright (c) 2015 Tobias Stoeckmann tob...@openbsd.org + * + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE

df: division by zero on invalid ext2fs

2015-03-01 Thread Tobias Stoeckmann
Hi, it is possible to trigger a floating point exception in df when it is used to retrieve information from a raw device with a broken ext2 file system. These are steps to prepare a file system with an invalid entry for e2fs_log_bsize (0xFF): $ dd if=/dev/zero of=ext2.fs bs=1K count=1440 #

tail: -r mem leak with non-regular files

2015-03-26 Thread Tobias Stoeckmann
Hi, tail -r has two memory leaks when handling non-regular files. You can easily see memory usage increasing with commands like $ mknod pipe p $ tail -r pipe pipe pipe /dev/null And then writing a large file into the pipe three times. top shows the memory usage of tail increasing with each

sort: another mkstemp use case

2015-04-01 Thread Tobias Stoeckmann
When creating a new temporary file name, use mkstemp instead of just taking a rather predictable path, which could even be a symlink by a malicious user (granted, that is very unlikely). Index: file.c === RCS file:

getusershell: possible overflow

2015-08-11 Thread Tobias Stoeckmann
Hi, I know this will be the third commit to fix the overflow situation in getusershell if /etc/shells is malformed. Hopefully it will be the last adjustment. I submitted a bug report to glibc devs after noticing that they use the same implementation like we do (more or less). Paul Pluzhnikov

Re: sed: better error handling

2015-10-26 Thread Tobias Stoeckmann
> Comments / oks? Looks much cleaner, okay for me.

Re: catopen/catgets: out of boundary access

2015-10-23 Thread Tobias Stoeckmann
On Fri, Oct 23, 2015 at 09:19:34PM +0200, Stefan Sperling wrote: > Now that this is committed, do you intend to audit the runes code as > well? :-) Hah, yeah that's the next logical step to do. Except you are faster than me, then I would probably okay it. ;)

sed: better error message

2015-10-25 Thread Tobias Stoeckmann
$ sed s/a/b/ /nofile sed: 0: /nofile: /nofile That message doesn't tell me a lot, let's better write strerror(errno) if fopen returns NULL: $ ./sed s/a/b/ /nofile sed: 0: /nofile: No such file or directory Index: main.c === RCS

Re: [PATCH] rcs: buf_free/rcsnum_free

2015-11-01 Thread Tobias Stoeckmann
On Sun, Nov 01, 2015 at 11:17:40AM +, Stuart Henderson wrote: > On 2015/11/01 08:03, Nicholas Marriott wrote: > > Some did for a while but it has some nasty bugs and nobody is working on > > fixing it. > > Some used it on amd64 for a while to avoid checkout failures due to > running into

Re: [PATCH] rcs: buf_free/rcsnum_free

2015-10-30 Thread Tobias Stoeckmann
On Fri, Oct 30, 2015 at 08:52:02AM +0800, Michael W. Bombardieri wrote: > Sorry. Here is new diff. Hopefully I haven't missed anything else. You missed OpenCVS, which shares the same code base. But is OpenCVS worth it anymore? Even a harsher question: Is it time to tedu it?

Re: unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-05 Thread Tobias Stoeckmann
On Thu, Nov 05, 2015 at 03:57:26PM +, Nicholas Marriott wrote: > I like this a lot. > > There are some trivial differences in the various xmalloc.h as well, and > I think you could make the style consistent within the files (eg "return > i" in xasprintf and xsnprintf). Oh yes, forgot to

Re: unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-07 Thread Tobias Stoeckmann
Here's an updated diff: - use "overflow" error message for snprintf and friends - use err instead of errx for out of memory conditions - if fatal() doesn't print error string, use ": %s", strerror(errno) Is this okay for ssh and tmux, which are out to be very portable? Nicholas mentioned that

Re: unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-07 Thread Tobias Stoeckmann
On Sat, Nov 07, 2015 at 05:57:55PM -0500, Ted Unangst wrote: > > Also, I'm seeing a couple "could not allocate memory" messages added to > > *snprintf() functions. They write to a supplied buffer, no? > > Good catch. Will update that one, thanks. > > > > + i = vsnprintf(str, len, fmt,

Re: cvs(1) simplification

2015-11-02 Thread Tobias Stoeckmann
I wouldn't call this definition readable: void cvs_ent_line_str(const char *name, char *rev, char *tstamp, char *opts, char *sticky, int isdir, int isremoved, char *buf, size_t len) So what about changing it to return allocated memory by itself, which would mean changing the internals from

unify xmalloc (was Re: [patch] cvs: retire xfree())

2015-11-05 Thread Tobias Stoeckmann
On Thu, Nov 05, 2015 at 09:50:48AM +, Nicholas Marriott wrote: > I don't know why cvs and rcs xmalloc.c has ended up so different. It's not just about cvs and rcs: /usr/src/usr.bin/cvs/xmalloc.c /usr/src/usr.bin/diff/xmalloc.c /usr/src/usr.bin/file/xmalloc.c /usr/src/usr.bin/rcs/xmalloc.c

nlist(3): out of boundary access

2015-10-15 Thread Tobias Stoeckmann
The library function nlist(3) does not properly validate parsed ELF binary files, which can lead to out of boundary accesses. Also, nlist will return -1 for stripped binary files, because eventually it will try to mmap 0 bytes. Instead of returning the amount of symbols we tried to look up, -1

Re: nlist(3): out of boundary access

2015-10-15 Thread Tobias Stoeckmann
On Thu, Oct 15, 2015 at 11:28:07AM -0600, Todd C. Miller wrote: > Those checks all look good. The only thing I had a question > about is the: > > len = strlen(sym); > > Would it be better to use memchr to search for the NUL terminator > to avoid going past the end? E.g. > > if

queue.3: missing curly bracket

2015-10-10 Thread Tobias Stoeckmann
That's what I get for copy out of a manual... The LIST_EMPTY example lacks an opening curly bracket. The other examples have it, so it's a pretty obvious fix. Index: queue.3 === RCS file: /cvs/src/share/man/man3/queue.3,v

patch: native ed support

2015-10-11 Thread Tobias Stoeckmann
ndex: ed.c === RCS file: ed.c diff -N ed.c --- /dev/null 1 Jan 1970 00:00:00 - +++ ed.c11 Oct 2015 09:43:59 - @@ -0,0 +1,335 @@ +/* $OpenBSD$ */ + +/* + * Copyright (c) 2015 Tobias Stoeckmann <tob...@openbsd.org> + * + * Permission to use, copy, modify

diff: use s/.// ed-substitution

2015-10-10 Thread Tobias Stoeckmann
GNU patch only allows s/.// as a regular expression in substitutions. Our diff implementation writes s/^\.\././ which is basically the same, because they are used to change ".." lines into ".". This is required if an ed-formatted diff tries to create a line that only has a dot in it. Normally,

Re: catopen/catgets: out of boundary access

2015-10-06 Thread Tobias Stoeckmann
By the way, this is the second version with miod's feedback. Time to send it to tech@ now, too. Fixed one issue due to missing braces and less ntohl() calls, which makes the code easier to read. Index: catopen.c === RCS file:

Re: patch: native ed support

2015-10-13 Thread Tobias Stoeckmann
ed.c .include Index: ed.c === RCS file: ed.c diff -N ed.c --- /dev/null 1 Jan 1970 00:00:00 - +++ ed.c13 Oct 2015 16:33:09 - @@ -0,0 +1,340 @@ +/* $OpenBSD$ */ + +/* + * Copyright (c) 2015 Tobias Stoeckmann <tob...@openbsd.org> + * + * Permission to use, copy, modify, a

  1   2   >