Control: tag -1 + patch On Tue, 26 Jun 2012 09:36:04 +0200, Moritz Muehlenhoff wrote:
> Please see > http://seclists.org/oss-sec/2012/q2/540 > http://seclists.org/oss-sec/2012/q2/544 Here's a patch for these issues. Since one of the included patches is rather huge, and I had to hand-edit all rejected hunks (the version in Debian is a bit *cough* old), I'd appreciate a review. Cheers, gregor -- .''`. Homepage: http://info.comodo.priv.at/ - OpenPGP key 0xBB3A68018649AA06 : :' : Debian GNU/Linux user, admin, and developer - http://www.debian.org/ `. `' Member of VIBE!AT & SPI, fellow of the Free Software Foundation Europe `- NP: Rolling Stones: Shame
diff -u viewvc-1.1.5/debian/changelog viewvc-1.1.5/debian/changelog --- viewvc-1.1.5/debian/changelog +++ viewvc-1.1.5/debian/changelog @@ -1,3 +1,15 @@ +viewvc (1.1.5-1.3) UNRELEASED; urgency=low + + * Non-maintainer upload. + * [SECURITY] Fix "CVE-2012-3356 / CVE-2012-3357": + - CVE-2012-3356: * security fix: complete authz support for remote SVN views + - CVE-2012-3357: * security fix: log msg leak in SVN revision view with + unreadable copy source + Add patches "CVE-2012-3356" and "CVE-2012-3357", taken from upstream svn. + (Closes: #679069) + + -- gregor herrmann <gre...@debian.org> Wed, 05 Sep 2012 17:53:17 +0200 + viewvc (1.1.5-1.2) unstable; urgency=low * Non-maintainer upload. diff -u viewvc-1.1.5/debian/patches/series viewvc-1.1.5/debian/patches/series --- viewvc-1.1.5/debian/patches/series +++ viewvc-1.1.5/debian/patches/series @@ -6,0 +7,2 @@ +CVE-2012-3357 +CVE-2012-3356 only in patch2: unchanged: --- viewvc-1.1.5.orig/debian/patches/CVE-2012-3357 +++ viewvc-1.1.5/debian/patches/CVE-2012-3357 @@ -0,0 +1,52 @@ +Bugs-Debian: http://bugs.debian.org/679069 +Description: fix CVE-2012-3357 +Last-Update: 2012-09-05 +Origin: upstream svn, cf. details below +Comment: this is the original commit from upstream, with only the path + adjusted and quilt refresh'd + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2758 + +Author cmpilato +Date 2012-06-15 16:05:47-0700 (2 months, 2 weeks ago) +Log message + +Fix a security issue: When a readable path is copied from an +unreadable one, Subversion will obscure the fact that the operation +was a copy (by removing copyfrom info) and will deem the log message +for the revision in which the copy occurred to be unreadable. ViewVC +was only doing the former bit; now it does the latter, too. + +* lib/vclib/svn/svn_repos.py + (LocalSubversionRepository._get_changed_paths): Set found_unreadable + when we have to hide a copyfrom path, too. + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2762 + +Author cmpilato +Date 2012-06-15 17:25:33-0700 (2 months, 2 weeks ago) +Log message + +Merge from trunk r2758, whose log message read like so: + + Fix a security issue: When a readable path is copied from an + unreadable one, Subversion will obscure the fact that the operation + was a copy (by removing copyfrom info) and will deem the log message + for the revision in which the copy occurred to be unreadable. ViewVC + was only doing the former bit; now it does the latter, too. + + * lib/vclib/svn/svn_repos.py + (LocalSubversionRepository._get_changed_paths): Set found_unreadable + when we have to hide a copyfrom path, too. + + +--- a/lib/vclib/svn/svn_repos.py ++++ b/lib/vclib/svn/svn_repos.py +@@ -648,6 +648,7 @@ + is_copy = 0 + change.base_path = None + change.base_rev = None ++ found_unreadable = 1 + changedpaths[path] = SVNChangedPath(path, rev, pathtype, + change.base_path, + change.base_rev, action, only in patch2: unchanged: --- viewvc-1.1.5.orig/debian/patches/CVE-2012-3356 +++ viewvc-1.1.5/debian/patches/CVE-2012-3356 @@ -0,0 +1,368 @@ +Bugs-Debian: http://bugs.debian.org/679069 +Description: fix CVE-2012-3356 +Last-Update: 2012-09-05 +Origin: upstream svn, cf. details below +Comment: this is the commit from upstream, with the path adjusted and quilt + refresh'd plus heavy manual backporting to the old version in Debian + because 5/13 hunks failed + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2755 + +Author cmpilato +Date 2012-06-13 14:57:38-0700 (2 months, 3 weeks ago) +Log message + +For issue #353, check path access in the code that backs the revision +log view for remote SVN repositories. + +* lib/vclib/svn/svn_ra.py + (LogCollector.__init__): Add 'access_check_func' parameter, stashed + away as a member variable. Initialize 'done' variable to False. + (LogCollector.add_log): Use access_check_func() to test for access. + If access is denied, set 'done' to True and ignore future + invocations. + (RemoteSubversionRepository.itemlog): Add _access_checker inner + function, passed as a callback to LogCollector.__init__(). + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2756 + +Author cmpilato +Date 2012-06-14 19:11:12-0700 (2 months, 3 weeks ago) +Log message + +For issue #353 ("Remote Subversion repositories not fully honoring +authz rules"): + +* lib/vclib/svn/svn_ra.py + (RemoteSubversionRepository.get_locations): Check authz on the + result of a get_locations lookup (being sure to use the original + path in the error message, if any). + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2757 + +Author cmpilato +Date 2012-06-14 19:17:55-0700 (2 months, 3 weeks ago) +Log message + +For issue #353, check path access in the annotation view code for +remote SVN repositories. + +* lib/vclib/svn/svn_ra.py + (RemoteSubversionRepository.annotate): Examine revision logs to + determine the legal annotation range we are allowed to request, + and use the revinfo cache to strip sensitive author/data info from + the annotation data. + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2759 + +Author cmpilato +Date 2012-06-15 16:07:22-0700 (2 months, 2 weeks ago) +Log message + +* lib/vclib/svn/svn_ra.py + (LogCollector.add_log): Allow the access_check_func to be None (and + avoid trying to call it in that case). + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2760 + +Author cmpilato +Date 2012-06-15 16:34:01-0700 (2 months, 2 weeks ago) +Log message + +Finish, I believe, issue #353 ("Remote Subversion repositories not +fully honoring authz rules"). In doing so, this makes the svn_ra +module the most accurate it has ever been, hopefully without too +terribly much extra cost. + +* lib/vclib/svn/svn_ra.py + (RemoteSubversionRepository.listdir): No longer check access here. + That's handled down in _get_dirents() now. + (RemoteSubversionRepository.dirlogs): Don't check dirent access here. + Just ensure that the returned entries are those which are + represented in the (filtered) set return from _get_dirents(). + Also, replace the revision metadata stored on each dirent with + values from the revinfo cache (which does authz sanitizing). + (RemoteSubversionRepository._get_dirents): Do authz-checking of + dirents here, and use _get_last_history_rev() to populate a useful + created_rev. + (RemoteSubversionRepository._get_last_history_rev): Because + Subversion's RA layer doesn't consider copy events when + determining an item's last-committed-rev, compensate for this with + a bounded log operation which does. This brings created-rev + parity with the svn_repos module (at the cost of the extra RA + work, but with the benefit of, you know, accuracy. + (RemoteSubversionRepository._revinfo_raw): Leave a TODO about a + future optimization. + (RemoteSubversionRepository._log_cb): Set found_unreadable when + sanitizing an unreadable copyfrom path, too. + (RemoteSubversionRepository.created_rev): Now just a thin wrapper + around the beefed-up _get_last_history_rev() function. + +http://viewvc.tigris.org/source/browse/viewvc?view=rev&revision=2761 + +Author cmpilato +Date 2012-06-15 17:03:57-0700 (2 months, 2 weeks ago) +Log message + +Merge the fixes for issue #353 ("Remote Subversion repositories not +fully honoring authz rules") from trunk. This merges r2755, r2756, +r2757, r2759, and r2760, which see for detail log revision information. + +--- a/lib/vclib/svn/svn_ra.py ++++ b/lib/vclib/svn/svn_ra.py +@@ -56,9 +56,8 @@ + + + class LogCollector: +- ### TODO: Make this thing authz-aware + +- def __init__(self, path, show_all_logs, lockinfo): ++ def __init__(self, path, show_all_logs, lockinfo, access_check_func): + # This class uses leading slashes for paths internally + if not path: + self.path = '/' +@@ -67,8 +66,12 @@ + self.logs = [] + self.show_all_logs = show_all_logs + self.lockinfo = lockinfo ++ self.access_check_func = access_check_func ++ self.done = False + + def add_log(self, paths, revision, author, date, message, pool): ++ if self.done: ++ return + # Changed paths have leading slashes + changed_paths = paths.keys() + changed_paths.sort(lambda a, b: _compare_paths(a, b)) +@@ -88,9 +91,13 @@ + if change.copyfrom_path: + this_path = change.copyfrom_path + self.path[len(changed_path):] + if self.show_all_logs or this_path: +- entry = Revision(revision, _datestr_to_date(date), author, message, None, +- self.lockinfo, self.path[1:], None, None) +- self.logs.append(entry) ++ if self.access_check_func is None \ ++ or self.access_check_func(self.path[1:], revision): ++ entry = Revision(revision, _datestr_to_date(date), author, message, None, ++ self.lockinfo, self.path[1:], None, None) ++ self.logs.append(entry) ++ else: ++ self.done = True + if this_path: + self.path = this_path + +@@ -229,7 +236,7 @@ + if self.itemtype(path_parts, rev) != vclib.DIR: # does auth-check + raise vclib.Error("Path '%s' is not a directory." % path) + rev = self._getrev(rev) +- entries = [ ] ++ entries = [] + dirents, locks = self._get_dirents(path, rev) + for name in dirents.keys(): + entry = dirents[name] +@@ -237,8 +244,9 @@ + kind = vclib.DIR + elif entry.kind == core.svn_node_file: + kind = vclib.FILE +- if vclib.check_path_access(self, path_parts + [name], kind, rev): +- entries.append(vclib.DirEntry(name, kind)) ++ else: ++ kind = None ++ entries.append(vclib.DirEntry(name, kind)) + return entries + + def dirlogs(self, path_parts, rev, entries, options): +@@ -249,9 +257,11 @@ + dirents, locks = self._get_dirents(path, rev) + for entry in entries: + entry_path_parts = path_parts + [entry.name] +- if not vclib.check_path_access(self, entry_path_parts, entry.kind, rev): ++ dirent = dirents.get(entry.name, None) ++ # dirents is authz-sanitized, so ensure the entry is found therein. ++ if dirent is None: + continue +- dirent = dirents[entry.name] ++ # Get authz-sanitized revision metadata. + entry.date, entry.author, entry.log, changes = \ + self.revinfo(dirent.created_rev) + entry.rev = str(dirent.created_rev) +@@ -275,9 +285,14 @@ + if locks.has_key(basename): + lockinfo = locks[basename].owner + ++ def _access_checker(check_path, check_rev): ++ return vclib.check_path_access(self, _path_parts(check_path), ++ path_type, check_rev) ++ + # It's okay if we're told to not show all logs on a file -- all + # the revisions should match correctly anyway. +- lc = LogCollector(path, options.get('svn_show_all_dir_logs', 0), lockinfo) ++ lc = LogCollector(path, options.get('svn_show_all_dir_logs', 0), ++ lockinfo, _access_checker) + + cross_copies = options.get('svn_cross_copies', 0) + log_limit = 0 +@@ -290,6 +305,10 @@ + revs.sort() + prev = None + for rev in revs: ++ # Swap out revision info with stuff from the cache (which is ++ # authz-sanitized). ++ rev.date, rev.author, rev.log, revprops, changes \ ++ = self.revinfo(rev.number) + rev.prev = prev + prev = rev + revs.reverse() +@@ -316,6 +335,14 @@ + rev = self._getrev(rev) + url = self._geturl(path) + ++ # Examine logs for the file to determine the oldest revision we are ++ # permitted to see. ++ revs = self.itemlog(path_parts, rev, vclib.SORTBY_REV, 0, 0, {}) ++ oldest_rev = revs[-1].number ++ ++ # Now calculate the annotation data. Note that we'll not ++ # inherently trust the provided author and date, because authz ++ # rules might necessitate that we strip that information out. + blame_data = [] + + def _blame_cb(line_no, revision, author, date, +@@ -323,10 +350,14 @@ + prev_rev = None + if revision > 1: + prev_rev = revision - 1 ++ if revision >= 0: ++ date, author, msg, revprops, changes = self.revinfo(revision) ++ else: ++ date = author = None + blame_data.append(vclib.Annotation(line, line_no+1, revision, prev_rev, + author, None)) + +- client.svn_client_blame(url, _rev2optrev(1), _rev2optrev(rev), ++ client.svn_client_blame(url, _rev2optrev(oldest_rev), _rev2optrev(rev), + _blame_cb, self.ctx) + + return blame_data, rev +@@ -392,32 +423,73 @@ + + def _get_dirents(self, path, rev): + """Return a 2-type of dirents and locks, possibly reading/writing +- from a local cache of that information.""" ++ from a local cache of that information. This functions performs ++ authz checks, stripping out unreadable dirents.""" + + dir_url = self._geturl(path) + if path: + key = str(rev) + '/' + path + else: + key = str(rev) ++ ++ # Ensure that the cache gets filled... + dirents_locks = self._dirent_cache.get(key) + if not dirents_locks: +- dirents, locks = list_directory(dir_url, _rev2optrev(rev), +- _rev2optrev(rev), 0, self.ctx) ++ tmp_dirents, locks = list_directory(dir_url, _rev2optrev(rev), ++ _rev2optrev(rev), 0, self.ctx) ++ dirents = {} ++ for name, dirent in tmp_dirents.items(): ++ dirent_parts = _path_parts(path) + [name] ++ kind = dirent.kind ++ if (kind == core.svn_node_dir or kind == core.svn_node_file) \ ++ and vclib.check_path_access(self, dirent_parts, ++ kind == core.svn_node_dir \ ++ and vclib.DIR or vclib.FILE, rev): ++ dirent.created_rev = self._get_last_history_rev(dirent_parts, rev) ++ dirents[name] = dirent + dirents_locks = [dirents, locks] + self._dirent_cache[key] = dirents_locks ++ ++ # ...then return the goodies from the cache. + return dirents_locks[0], dirents_locks[1] + + def _get_last_history_rev(self, path_parts, rev): ++ """Return the last interesting revision equal to or older than REV ++ in the history of PATH_PARTS.""" ++ ++ path = self._getpath(path_parts) + url = self._geturl(self._getpath(path_parts)) + optrev = _rev2optrev(rev) ++ ++ # Get the last-changed-rev. + revisions = [] + def _info_cb(path, info, pool, retval=revisions): + revisions.append(info.last_changed_rev) + client.svn_client_info(url, optrev, optrev, _info_cb, 0, self.ctx) +- return revisions[0] ++ last_changed_rev = revisions[0] ++ ++ # Now, this object might not have been directly edited since the ++ # last-changed-rev, but it might have been the child of a copy. ++ # To determine this, we'll run a potentially no-op log between ++ # LAST_CHANGED_REV and REV. ++ lc = LogCollector(path, 1, None, None) ++ client_log(url, optrev, _rev2optrev(last_changed_rev), 1, 0, ++ lc.add_log, self.ctx) ++ revs = lc.logs ++ if revs: ++ revs.sort() ++ return revs[0].number ++ else: ++ return last_changed_rev + + def _revinfo_raw(self, rev): + # return 5-tuple (date, author, message, changes) ++ ++ ### TODO: This function and its related cache would benefit from ++ ### optimizations such as what the matching svn_repos functions ++ ### have, where the 'changes' information is only fully ++ ### calculated/authz-sanitized when the caller actually needs it. ++ + optrev = _rev2optrev(rev) + revs = [] + +@@ -457,10 +529,11 @@ + if vclib.check_path_access(self, parts, vclib.FILE, revision): + if is_copy and base_path and (base_path != path): + parts = _path_parts(base_path) +- if vclib.check_path_access(self, parts, vclib.FILE, base_rev): ++ if not vclib.check_path_access(self, parts, vclib.FILE, base_rev): + is_copy = 0 + base_path = None + base_rev = None ++ found_unreadable = 1 + changes.append(SVNChangedPath(path, revision, pathtype, base_path, + base_rev, action, is_copy, 0, 0)) + found_readable = 1 +@@ -495,23 +568,15 @@ + old_path = results[old_rev] + except KeyError: + raise vclib.ItemNotFound(path) +- +- return _cleanup_path(old_path) ++ old_path = _cleanup_path(old_path) ++ old_path_parts = _path_parts(old_path) ++ # Check access (lying about path types) ++ if not vclib.check_path_access(self, old_path_parts, vclib.FILE, old_rev): ++ raise vclib.ItemNotFound(path) ++ return old_path + + def created_rev(self, path, rev): +- # NOTE: We can't use svn_client_propget here because the +- # interfaces in that layer strip out the properties not meant for +- # human consumption (such as svn:entry:committed-rev, which we are +- # using here to get the created revision of PATH@REV). +- kind = ra.svn_ra_check_path(self.ra_session, path, rev) +- if kind == core.svn_node_none: +- raise vclib.ItemNotFound(_path_parts(path)) +- elif kind == core.svn_node_dir: +- props = get_directory_props(self.ra_session, path, rev) +- elif kind == core.svn_node_file: +- fetched_rev, props = ra.svn_ra_get_file(self.ra_session, path, rev, None) +- return int(props.get(core.SVN_PROP_ENTRY_COMMITTED_REV, +- SVN_INVALID_REVNUM)) ++ return self._get_last_history_rev(_path_parts(path), rev) + + def last_rev(self, path, peg_revision, limit_revision=None): + """Given PATH, known to exist in PEG_REVISION, find the youngest
signature.asc
Description: Digital signature
