Ubuntu
apport package

Merge lp:~mwhudson/ubuntu/focal/apport/lp-1854237 into lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu

  1. Focal (20.04)
  2. lp-1854237
  3. Merge into ubuntu
Proposed by Michael Hudson-Doyle
Status: Merged
Merged at revision: 2725
Proposed branch: lp:~mwhudson/ubuntu/focal/apport/lp-1854237
Merge into: lp:~ubuntu-core-dev/ubuntu/focal/apport/ubuntu
Diff against target: 220 lines (+49/-47)
6 files modified
apport/report.py (+19/-10)
data/apport (+13/-6)
debian/changelog (+12/-0)
etc/init.d/apport (+2/-2)
test/test_report.py (+3/-1)
test/test_signal_crashes.py (+0/-28)
To merge this branch: bzr merge lp:~mwhudson/ubuntu/focal/apport/lp-1854237
Reviewer Review Type Date Requested Status
Brian Murray Approve
Review via email: mp+376374@code.launchpad.net

Commit message

* Fix autopkgtest failures since recent security update: (LP: #1854237)
    - Fix regression in creating report for crashing setuid process by getting
      kernel to tell us the executable path rather than reading
      /proc/[pid]/exe.
    - Fix deletion of partially written core files.
    - Fix test_get_logind_session to use new API.
    - Restore add_proc_info raising ValueError for a dead process.
    - Delete test_lock_symlink, no longer applicable now that the lock is
      created in a directory only root can write to.

To post a comment you must log in.
lp:~mwhudson/ubuntu/focal/apport/lp-1854237 updated
2728. By Michael Hudson-Doyle

Restore add_proc_info raising ValueError for a dead process.

2729. By Michael Hudson-Doyle

Delete test_lock_symlink, no longer applicable now that the lock is
created in a directory only root can write to.

2730. By Michael Hudson-Doyle

fix test_get_logind_session

apport.Report.get_logind_session now takes a proc_pid_fd, not a pid

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

This branch now passes the autopkgtests in my testing \o/

Revision history for this message
Brian Murray (brian-murray) wrote :

This looks great! Thanks for taking the time to dig into it and sort it out properly.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'apport/report.py'
2--- apport/report.py 2019-12-03 03:18:16 +0000
3+++ apport/report.py 2019-12-05 02:42:15 +0000
4@@ -541,7 +541,15 @@
5 if not self.pid:
6 self.pid = int(pid)
7 pid = str(pid)
8- proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
9+ try:
10+ proc_pid_fd = os.open('/proc/%s' % pid, os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
11+ except (PermissionError, OSError, FileNotFoundError) as e:
12+ if e.errno in (errno.EPERM, errno.EACCES):
13+ raise ValueError('not accessible')
14+ if e.errno == errno.ENOENT:
15+ raise ValueError('invalid process')
16+ else:
17+ raise
18
19 try:
20 self['ProcCwd'] = os.readlink('cwd', dir_fd=proc_pid_fd)
21@@ -551,15 +559,16 @@
22 self['ProcStatus'] = _read_file('status', dir_fd=proc_pid_fd)
23 self['ProcCmdline'] = _read_file('cmdline', dir_fd=proc_pid_fd).rstrip('\0')
24 self['ProcMaps'] = _read_maps(proc_pid_fd)
25- try:
26- self['ExecutablePath'] = os.readlink('exe', dir_fd=proc_pid_fd)
27- except (PermissionError, OSError, FileNotFoundError) as e:
28- if e.errno in (errno.EPERM, errno.EACCES):
29- raise ValueError('not accessible')
30- if e.errno == errno.ENOENT:
31- raise ValueError('invalid process')
32- else:
33- raise
34+ if 'ExecutablePath' not in self:
35+ try:
36+ self['ExecutablePath'] = os.readlink('exe', dir_fd=proc_pid_fd)
37+ except (PermissionError, OSError, FileNotFoundError) as e:
38+ if e.errno in (errno.EPERM, errno.EACCES):
39+ raise ValueError('not accessible')
40+ if e.errno == errno.ENOENT:
41+ raise ValueError('invalid process')
42+ else:
43+ raise
44 for p in ('rofs', 'rwfs', 'squashmnt', 'persistmnt'):
45 if self['ExecutablePath'].startswith('/%s/' % p):
46 self['ExecutablePath'] = self['ExecutablePath'][len('/%s' % p):]
47
48=== modified file 'data/apport'
49--- data/apport 2019-12-03 03:18:16 +0000
50+++ data/apport 2019-12-05 02:42:15 +0000
51@@ -211,7 +211,7 @@
52 if limit > 0 and core_size > limit:
53 error_log('aborting core dump writing, size %i exceeds current limit' % core_size)
54 os.close(core_file)
55- os.unlink(core_path)
56+ os.unlink(core_path, dir_fd=cwd)
57 return
58 error_log('writing core dump %s of size %i' % (core_path, core_size))
59 os.write(core_file, r['CoreDump'])
60@@ -227,17 +227,16 @@
61 if limit > 0 and written > limit:
62 error_log('aborting core dump writing, size exceeds current limit %i' % limit)
63 os.close(core_file)
64- os.unlink(core_path)
65+ os.unlink(core_path, dir_fd=cwd)
66 return
67 if os.write(core_file, block) != size:
68 error_log('aborting core dump writing, could not write')
69 os.close(core_file)
70- os.unlink(core_path)
71+ os.unlink(core_path, dir_fd=cwd)
72 return
73 block = os.read(0, 1048576)
74
75 os.close(core_file)
76- return core_path
77
78
79 def usable_ram():
80@@ -340,7 +339,7 @@
81 def parse_arguments():
82 parser = argparse.ArgumentParser(epilog="""
83 Alternatively, the following command line is understood for legacy hosts:
84- <pid> <signal number> <core file ulimit> <dump mode> [global pid]
85+ <pid> <signal number> <core file ulimit> <dump mode> [global pid] [exe path]
86 """)
87
88 # TODO: Use type=int
89@@ -349,6 +348,7 @@
90 parser.add_argument("-c", "--core-ulimit", help="core ulimit (%%c)")
91 parser.add_argument("-d", "--dump-mode", help="dump mode (%%d)")
92 parser.add_argument("-P", "--global-pid", nargs='?', help="pid in root namespace (%%P)")
93+ parser.add_argument("-E", "--executable-path", nargs='?', help="path of executable (%%E)")
94
95 options, rest = parser.parse_known_args()
96
97@@ -356,7 +356,7 @@
98 for arg in rest:
99 error_log("Unknown argument: %s", arg)
100
101- elif len(rest) in (4, 5):
102+ elif len(rest) in (4, 5, 6):
103 # Translate legacy command line
104 options.pid = rest[0]
105 options.signal_number = rest[1]
106@@ -366,6 +366,10 @@
107 options.global_pid = rest[4]
108 except IndexError:
109 options.global_pid = None
110+ try:
111+ options.exe_path = rest[5].replace('!', '/')
112+ except IndexError:
113+ options.exe_path = None
114 else:
115 parser.print_usage()
116 sys.exit(1)
117@@ -599,6 +603,9 @@
118 # We already need this here to figure out the ExecutableName (for scripts,
119 # etc).
120
121+ if options.exe_path is not None and os.path.exists(options.exe_path):
122+ info['ExecutablePath'] = options.exe_path
123+
124 euid = os.geteuid()
125 egid = os.getegid()
126 try:
127
128=== modified file 'debian/changelog'
129--- debian/changelog 2019-12-04 21:24:04 +0000
130+++ debian/changelog 2019-12-05 02:42:15 +0000
131@@ -1,8 +1,20 @@
132 apport (2.20.11-0ubuntu13) UNRELEASED; urgency=medium
133
134+ [ Brian Murray ]
135 * Create additional symlinks to the source_linux.py apport package hook for
136 many OEM kernels. Thanks to You-Sheng Yang for the patch. (LP: #1847967)
137
138+ [ Michael Hudson-Doyle ]
139+ * Fix autopkgtest failures since recent security update: (LP: #1854237)
140+ - Fix regression in creating report for crashing setuid process by getting
141+ kernel to tell us the executable path rather than reading
142+ /proc/[pid]/exe.
143+ - Fix deletion of partially written core files.
144+ - Fix test_get_logind_session to use new API.
145+ - Restore add_proc_info raising ValueError for a dead process.
146+ - Delete test_lock_symlink, no longer applicable now that the lock is
147+ created in a directory only root can write to.
148+
149 -- Brian Murray <brian@ubuntu.com> Wed, 04 Dec 2019 13:21:43 -0800
150
151 apport (2.20.11-0ubuntu12) focal; urgency=medium
152
153=== modified file 'etc/init.d/apport'
154--- etc/init.d/apport 2019-05-16 19:37:48 +0000
155+++ etc/init.d/apport 2019-12-05 02:42:15 +0000
156@@ -52,9 +52,9 @@
157
158 # Old compatibility mode, switch later to second one
159 if true; then
160- echo "|$AGENT %p %s %c %d %P" > /proc/sys/kernel/core_pattern
161+ echo "|$AGENT %p %s %c %d %P %E" > /proc/sys/kernel/core_pattern
162 else
163- echo "|$AGENT -p%p -s%s -c%c -d%d -P%P" > /proc/sys/kernel/core_pattern
164+ echo "|$AGENT -p%p -s%s -c%c -d%d -P%P -E%E" > /proc/sys/kernel/core_pattern
165 fi
166 echo 2 > /proc/sys/fs/suid_dumpable
167 }
168
169=== modified file 'test/test_report.py'
170--- test/test_report.py 2019-05-17 17:51:10 +0000
171+++ test/test_report.py 2019-12-05 02:42:15 +0000
172@@ -2274,7 +2274,9 @@
173 self.assertEqual(expected, pr.crash_signature())
174
175 def test_get_logind_session(self):
176- ret = apport.Report.get_logind_session(os.getpid())
177+ proc_pid_fd = os.open('/proc/%s' % os.getpid(), os.O_RDONLY | os.O_PATH | os.O_DIRECTORY)
178+ self.addCleanup(os.close, proc_pid_fd)
179+ ret = apport.Report.get_logind_session(proc_pid_fd)
180 if ret is None:
181 # ensure that we don't run under logind, and thus the None is
182 # justified
183
184=== modified file 'test/test_signal_crashes.py'
185--- test/test_signal_crashes.py 2018-05-09 23:30:27 +0000
186+++ test/test_signal_crashes.py 2019-12-05 02:42:15 +0000
187@@ -192,34 +192,6 @@
188 os.kill(test_proc2, 9)
189 os.waitpid(test_proc2, 0)
190
191- def test_lock_symlink(self):
192- '''existing .lock file as dangling symlink does not create the file
193-
194- This would be a vulnerability, as users could overwrite system files.
195- '''
196- # prepare a symlink trap
197- lockpath = os.path.join(self.report_dir, '.lock')
198- trappath = os.path.join(self.report_dir, '0wned')
199- os.symlink(trappath, lockpath)
200-
201- # now call apport
202- test_proc = self.create_test_process()
203-
204- try:
205- app = subprocess.Popen([apport_path, str(test_proc), '42', '0', '1'],
206- stdin=subprocess.PIPE, stderr=subprocess.PIPE)
207- app.stdin.write(b'boo')
208- app.stdin.close()
209-
210- self.assertNotEqual(app.wait(), 0, app.stderr.read())
211- app.stderr.close()
212- finally:
213- os.kill(test_proc, 9)
214- os.waitpid(test_proc, 0)
215-
216- self.assertEqual(self.get_temp_all_reports(), [])
217- self.assertFalse(os.path.exists(trappath))
218-
219 def test_unpackaged_binary(self):
220 '''unpackaged binaries do not create a report'''
221

Subscribers

People subscribed via source and target branches