Commit Graph

217 Commits

Author SHA1 Message Date
Kristof Provost
6273ba66f2 pf: Avoid integer overflow issues by using mallocarray() iso. malloc()
pfioctl() handles several ioctl that takes variable length input, these
include:
- DIOCRADDTABLES
- DIOCRDELTABLES
- DIOCRGETTABLES
- DIOCRGETTSTATS
- DIOCRCLRTSTATS
- DIOCRSETTFLAGS

All of them take a pfioc_table struct as input from userland. One of
its elements (pfrio_size) is used in a buffer length calculation.
The calculation contains an integer overflow which if triggered can lead
to out of bound reads and writes later on.

Reported by:	Ilja Van Sprundel <ivansprundel@ioactive.com>
2018-01-07 13:35:15 +00:00
Kristof Provost
9d671fee3a pf: Allow the module to be unloaded
pf can now be safely unloaded. Most of this code is exercised on vnet
jail shutdown.

Don't block unloading.
2017-12-31 16:18:13 +00:00
Kristof Provost
5d0020d6d7 pf: Clean all fragments on shutdown
When pf is unloaded, or a vnet jail using pf is stopped we need to
ensure we clean up all fragments, not just the expired ones.
2017-12-31 10:01:31 +00:00
Pedro F. Giffuni
6e778a7efd SPDX: license IDs for some ISC-related files. 2017-12-08 15:57:29 +00:00
Pedro F. Giffuni
8820ecc040 SPDX: Fix some cases wrongly attributed to MIT.
In the cases of BSD-style license variants without clauses, use 0BSD for
the time being in lack of a better description.
2017-11-30 15:10:11 +00:00
Pedro F. Giffuni
fe267a5590 sys: general adoption of SPDX licensing ID tags.
Mainly focus on files that use BSD 2-Clause license, however the tool I
was using misidentified many licenses so this was mostly a manual - error
prone - task.

The Software Package Data Exchange (SPDX) group provides a specification
to make it easier for automated tools to detect and summarize well known
opensource licenses. We are gradually adopting the specification, noting
that the tags are considered only advisory and do not, in any way,
superceed or replace the license texts.

No functional change intended.
2017-11-27 15:23:17 +00:00
Pedro F. Giffuni
51369649b0 sys: further adoption of SPDX licensing ID tags.
Mainly focus on files that use BSD 3-Clause license.

The Software Package Data Exchange (SPDX) group provides a specification
to make it easier for automated tools to detect and summarize well known
opensource licenses. We are gradually adopting the specification, noting
that the tags are considered only advisory and do not, in any way,
superceed or replace the license texts.

Special thanks to Wind River for providing access to "The Duke of
Highlander" tool: an older (2014) run over FreeBSD tree was useful as a
starting point.
2017-11-20 19:43:44 +00:00
Kristof Provost
7f3ad01804 pf_get_sport(): Prevent possible endless loop when searching for an unused nat port
This is an import of Alexander Bluhm's OpenBSD commit r1.60,
the first chunk had to be modified because on OpenBSD the
'cut' declaration is located elsewhere.

Upstream report by Jingmin Zhou:
https://marc.info/?l=openbsd-pf&m=150020133510896&w=2

OpenBSD commit message:
 Use a 32 bit variable to detect integer overflow when searching for
 an unused nat port.  Prevents a possible endless loop if high port
 is 65535 or low port is 0.
 report and analysis Jingmin Zhou; OK sashan@ visa@
Quoted from: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/pf_lb.c

PR:		221201
Submitted by:	Fabian Keil <fk@fabiankeil.de>
Obtained from:  OpenBSD via ElectroBSD
MFC after:	1 week
2017-08-08 21:09:26 +00:00
Kristof Provost
b7ae43552b pf: Fix vnet purging
pf_purge_thread() breaks up the work of iterating all states (in
pf_purge_expired_states()) and tracks progress in the idx variable.

If multiple vnets exist this results in pf_purge_thread() only calling
pf_purge_expired_states() for part of the states (the first part of the
first vnet, second part of the second vnet and so on).
Combined with the mark-and-sweep approach to cleaning up old rules (in
V_pf_unlinked_rules) that resulted in pf freeing rules that were still
referenced by states. This in turn caused panics when pf_state_expires()
encounters that state and attempts to access the rule.

We need to track the progress per vnet, not globally, so idx is moved
into a per-vnet V_pf_purge_idx.

PR:		219251
Sponsored by:	Hackathon Essen 2017
2017-07-09 17:56:39 +00:00
Kristof Provost
468cefa22e pf: Fix vnet initialisation
When running the vnet init code (pf_load_vnet()) we used to iterate over
all vnets, marking them as unhooked.
This is incorrect and leads to panics if pf is unloaded, as the unload
code does not unregister the pfil hooks (because the vnet is marked as
unhooked).

There's no need or reason to touch other vnets during initialisation.
Their pf_load_vnet() function will be triggered, which handles all
required initialisation.

Reviewed by:	zec, gnn
Differential Revision:	https://reviews.freebsd.org/D10592
2017-05-07 14:33:58 +00:00
Kristof Provost
64c79ee733 pf: Fix panic on unload
vnet_pf_uninit() is called through vnet_deregister_sysuninit() and
linker_file_unload() when the pf module is unloaded. This is executed
after pf_unload() so we end up trying to take locks which have been
destroyed already.

Move pf_unload() to a separate SYSUNINIT() to ensure it's called after
all the vnet_pf_uninit() calls.

Differential Revision:	https://reviews.freebsd.org/D10025
2017-05-03 20:56:54 +00:00
Marko Zec
1e9e374199 Fix VNET leakages in PF by V_irtualizing pfr_ktables and friends.
Apparently this resolves a PF-triggered panic when destroying VNET jails.

Submitted by:	Peter Blok <peter.blok@bsd4all.org>
Reviewed by:	kp
2017-04-25 08:34:39 +00:00
Marko Zec
3a36ee404f Since curvnet is already properly set on entry to event handlers,
there's no need to override it, particularly not unconditionally with
vnet0.

Submitted by:	Peter Blok <peter.blok@bsd4all.org>
Reviewed by:	kp
2017-04-25 08:30:28 +00:00
Kristof Provost
00eab743ab pf: Fix possible incorrect IPv6 fragmentation
When forwarding pf tracks the size of the largest fragment in a fragmented
packet, and refragments based on this size.
It failed to ensure that this size was a multiple of 8 (as is required for all
but the last fragment), so it could end up generating incorrect fragments.

For example, if we received an 8 byte and 12 byte fragment pf would emit a first
fragment with 12 bytes of payload and the final fragment would claim to be at
offset 8 (not 12).

We now assert that the fragment size is a multiple of 8 in ip6_fragment(), so
other users won't make the same mistake.

Reported by:	Antonios Atlasis <aatlasis at secfu net>
MFC after:	3 days
2017-04-20 09:05:53 +00:00
Kristof Provost
4e261006a1 pf: Also clear limit counters
The "pfctl -F info" command didn't clear the limit counters ( as shown in the
"pfctl -vsi" output).

Submitted by:	Max <maximos@als.nnov.ru>
2017-04-18 20:07:21 +00:00
Gleb Smirnoff
9f5efe718f Fix potential NULL deref.
Found by:	PVS Studio
2017-04-14 01:56:15 +00:00
Kristof Provost
3601d25181 pf: Fix leak of pf_state_keys
If we hit the state limit we returned from pf_create_state() without cleaning
up.

PR:		217997
Submitted by:	Max <maximos@als.nnov.ru>
MFC after:	1 week
2017-04-01 12:22:34 +00:00
Kristof Provost
2f8fb3a868 pf: Fix possible shutdown race
Prevent possible races in the pf_unload() / pf_purge_thread() shutdown
code. Lock the pf_purge_thread() with the new pf_end_lock to prevent
these races.

Use a shared/exclusive lock, as we need to also acquire another sx lock
(VNET_LIST_RLOCK). It's fine for both pf_purge_thread() and pf_unload()
to sleep,

Pointed out by: eri, glebius, jhb
Differential Revision:	https://reviews.freebsd.org/D10026
2017-03-22 21:18:18 +00:00
Kristof Provost
08ef4ddb0f pf: Fix rule evaluation after inet6 route-to
In pf_route6() we re-run the ruleset with PF_FWD if the packet goes out
of a different interface. pf_test6() needs to know that the packet was
forwarded (in case it needs to refragment so it knows whether to call
ip6_output() or ip6_forward()).

This lead pf_test6() to try to evaluate rules against the PF_FWD
direction, which isn't supported, so it needs to treat PF_FWD as PF_OUT.
Once fwdir is set correctly the correct output/forward function will be
called.

PR:		217883
Submitted by:	Kajetan Staszkiewicz
MFC after:	1 week
Sponsored by:	InnoGames GmbH
2017-03-19 03:06:09 +00:00
Kristof Provost
5c172e7059 pf: Fix memory leak on vnet shutdown or unload
Rules are unlinked in shutdown_pf(), so we must call
pf_unload_vnet_purge(), which frees unlinked rules, after that, not
before.

Reviewed by:	eri, bz
Differential Revision:	https://reviews.freebsd.org/D10040
2017-03-18 01:37:20 +00:00
Kristof Provost
2a57d24bd1 pf: Fix incorrect rw_sleep() in pf_unload()
When we unload we don't hold the pf_rules_lock, so we cannot call rw_sleep()
with it, because it would release a lock we do not hold. There's no need for the
lock either, so we can just tsleep().

While here also make the same change in pf_purge_thread(), because it explicitly
takes the lock before rw_sleep() and then immediately releases it afterwards.
2017-03-12 05:42:57 +00:00
Kristof Provost
f618201314 pf: Do not lose the VNET lock when ending the purge thread
When the pf_purge_thread() exits it must make sure to release the
VNET_LIST_RLOCK it still holds.
kproc_exit() does not return.
2017-03-12 05:00:04 +00:00
Kristof Provost
98a9874f7b pf: Fix a crash in low-memory situations
If the call to pf_state_key_clone() in pf_get_translation() fails (i.e. there's
no more memory for it) it frees skp. This is wrong, because skp is a
pf_state_key **, so we need to free *skp, as is done later in the function.
Getting it wrong means we try to free a stack variable of the calling
pf_test_rule() function, and we panic.
2017-03-06 23:41:23 +00:00
Eric van Gyzen
643faabe0d pf: use inet_ntoa_r() instead of inet_ntoa(); maybe fix IPv6 OS fingerprinting
inet_ntoa() cannot be used safely in a multithreaded environment
because it uses a static local buffer. Instead, use inet_ntoa_r()
with a buffer on the caller's stack.

This code had an INET6 conditional before this commit, but opt_inet6.h
was not included, so INET6 was never defined.  Apparently, pf's OS
fingerprinting hasn't worked with IPv6 for quite some time.
This commit might fix it, but I didn't test that.

Reviewed by:	gnn, kp
MFC after:	2 weeks
Relnotes:	yes (if I/someone can test pf OS fingerprinting with IPv6)
Sponsored by:	Dell EMC
Differential Revision:	https://reviews.freebsd.org/D9625
2017-02-16 20:44:44 +00:00
Gleb Smirnoff
164aa3ce5e Fix indentantion in pf_purge_thread(). No functional change. 2017-01-30 22:47:48 +00:00
Luiz Otavio O Souza
a5c1a50a26 Do not run the pf purge thread while the VNET variables are not
initialized, this can cause a divide by zero (if the VNET initialization
takes to long to complete).

Obtained from:	pfSense
MFC after:	2 weeks
Sponsored by:	Rubicon Communications, LLC (Netgate)
2017-01-29 02:17:52 +00:00
Marcel Moolenaar
aa8c6a6dca Improve upon r309394
Instead of taking an extra reference to deal with pfsync_q_ins()
and pfsync_q_del() taken and dropping a reference (resp,) make
it optional of those functions to take or drop a reference by
passing an extra argument.

Submitted by:	glebius@
2016-12-10 03:31:38 +00:00
Gleb Smirnoff
296d65b7a9 Backout accidentially leaked in r309746 not yet reviewed patch :( 2016-12-09 18:00:45 +00:00
Gleb Smirnoff
3cbee8caa1 Use counter_ratecheck() in the ICMP rate limiting.
Together with:	rrs, jtl
2016-12-09 17:59:15 +00:00
Kristof Provost
c3e14afc18 pflog: Correctly initialise subrulenr
subrulenr is considered unset if it's set to -1, not if it's set to 1.
See contrib/tcpdump/print-pflog.c pflog_print() for a user.

This caused incorrect pflog output (tcpdump -n -e -ttt -i pflog0):
  rule 0..16777216(match)
instead of the correct output of
  rule 0/0(match)

PR:		214832
Submitted by:	andywhite@gmail.com
2016-12-05 21:52:10 +00:00
Marcel Moolenaar
d6d35f1561 Fix use-after-free bugs in pfsync(4)
Use after free happens for state that is deleted. The reference
count is what prevents the state from being freed. When the
state is dequeued, the reference count is dropped and the memory
freed. We can't dereference the next pointer or re-queue the
state.

MFC after:	1 week
Differential Revision:	https://reviews.freebsd.org/D8671
2016-12-02 06:15:59 +00:00
Kristof Provost
1f4955785d pf: port extended DSCP support from OpenBSD
Ignore the ECN bits on 'tos' and 'set-tos' and allow to use
DCSP names instead of having to embed their TOS equivalents
as plain numbers.

Obtained from:	OpenBSD
Sponsored by:	OPNsense
Differential Revision:	https://reviews.freebsd.org/D8165
2016-10-13 20:34:44 +00:00
Kristof Provost
813196a11a pf: remove fastroute tag
The tag fastroute came from ipf and was removed in OpenBSD in 2011. The code
allows to skip the in pfil hooks and completely removes the out pfil invoke,
albeit looking up a route that the IP stack will likely find on its own.
The code between IPv4 and IPv6 is also inconsistent and marked as "XXX"
for years.

Submitted by:	Franco Fichtner <franco@opnsense.org>
Differential Revision:	https://reviews.freebsd.org/D8058
2016-10-04 19:35:14 +00:00
Kevin Lo
c7641cd18d Remove ifa_list, use ifa_link (structure field) instead.
While here, prefer if_addrhead (FreeBSD) to if_addrlist (BSD compat) naming
for the interface address list in sctp_bsd_addr.c

Reviewed by:	tuexen
Differential Revision:	https://reviews.freebsd.org/D8051
2016-09-28 13:29:11 +00:00
Kristof Provost
0df377cbb8 pf: Add missing byte-order swap to pf_match_addr_range
Without this, rules using address ranges (e.g. "10.1.1.1 - 10.1.1.5") did not
match addresses correctly on little-endian systems.

PR:		211796
Obtained from:	OpenBSD (sthen)
MFC after:	3 days
2016-08-15 12:13:14 +00:00
Kristof Provost
aa7cac58c6 pf: Map hook returns onto the correct error values
pf returns PF_PASS, PF_DROP, ... in the netpfil hooks, but the hook callers
expect to get E<foo> error codes.
Map the returns values. A pass is 0 (everything is OK), anything else means
pf ate the packet, so return EACCES, which tells the stack not to emit an ICMP
error message.

PR:	207598
2016-07-09 12:17:01 +00:00
Bjoern A. Zeeb
a8fc1b786d The void isn't void.
Unbreak sparc64 and powerpc builds.

Approved by:	re (gjb)
Sponsored by:	The FreeBSD Foundation
MFC after:	12 days
2016-06-24 11:53:12 +00:00
Bjoern A. Zeeb
66c00e9efb Proerply virtualize pfsync for bringup after pf is initialized and
teardown of VNETs once pf(4) has been shut down.
Properly split resources into VNET_SYS(UN)INITs and one time module
loading.
While here cover the INET parts in the uninit callpath with proper
#ifdefs.

Approved by:	re (gjb)
Obtained from:  projects/vnet
MFC after:      2 weeks
Sponsored by:   The FreeBSD Foundation
2016-06-23 22:31:44 +00:00
Bjoern A. Zeeb
7d7751a071 Make sure pflog is attached after pf is initializaed so we can
borrow pf's lock, and also make sure pflog goes after pf is gone
in order to avoid callouts in VNETs to an already freed instance.

Reported by:    Ivan Klymenko, Johan Hendriks  on current@ today
Obtained from:  projects/vnet
Sponsored by:   The FreeBSD Foundation
MFC after:      13 days
Approved by:	re (gjb)
2016-06-23 22:31:10 +00:00
Bjoern A. Zeeb
a8e8c57443 PFSTATE_NOSYNC goes onto state_flags, not sync_state;
this prevents: panic: pfsync_delete_state: unexpected sync state 8

Reviewed by:		kp
Approved by:		re (gjb)
MFC after:		2 weeks
Sponsored by:		The FreeBSD Foundation
Differential Revision:	https://reviews.freebsd.org/D6942
2016-06-23 21:42:43 +00:00
Bjoern A. Zeeb
a0429b5459 Update pf(4) and pflog(4) to survive basic VNET testing, which includes
proper virtualisation, teardown, avoiding use-after-free, race conditions,
no longer creating a thread per VNET (which could easily be a couple of
thousand threads), gracefully ignoring global events (e.g., eventhandlers)
on teardown, clearing various globally cached pointers and checking
them before use.

Reviewed by:		kp
Approved by:		re (gjb)
Sponsored by:		The FreeBSD Foundation
MFC after:		2 weeks
Differential Revision:	https://reviews.freebsd.org/D6924
2016-06-23 21:34:38 +00:00
Bjoern A. Zeeb
8147948e19 Import a fix for and old security issue (CVE-2010-3830) in pf which
was not relevant to FreeBSD as only root could open /dev/pf by default.
With VIMAGE this is will longer be the case.  As pf(4) starts to
be supported with VNETs 3rd party users may open /dev/pf inside the
virtual jail instance; thus we need to address this issue after all.
While OpenBSD largely rewrote code parts for the fix [1], and it's
unclear what Apple [3] did, import the minimal fix from NetBSD [2].

[1] http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/pf_ioctl.c.diff?r1=1.235&r2=1.236
[2] http://mail-index.netbsd.org/source-changes/2011/01/19/msg017518.html
[3] https://support.apple.com/en-gb/HT202154

Obtained from:		http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dist/pf/net/pf_ioctl.c.diff?r1=1.42&r2=1.43&only_with_tag=MAIN
MFC After:		2 weeks
Approved by:		re (gjb)
Sponsored by:		The FreeBSD Foundation
Security:		CVE-2010-3830
2016-06-23 05:41:46 +00:00
Bjoern A. Zeeb
89856f7e2d Get closer to a VIMAGE network stack teardown from top to bottom rather
than removing the network interfaces first. This change is rather larger
and convoluted as the ordering requirements cannot be separated.

Move the pfil(9) framework to SI_SUB_PROTO_PFIL, move Firewalls and
related modules to their own SI_SUB_PROTO_FIREWALL.
Move initialization of "physical" interfaces to SI_SUB_DRIVERS,
move virtual (cloned) interfaces to SI_SUB_PSEUDO.
Move Multicast to SI_SUB_PROTO_MC.

Re-work parts of multicast initialisation and teardown, not taking the
huge amount of memory into account if used as a module yet.

For interface teardown we try to do as many of them as we can on
SI_SUB_INIT_IF, but for some this makes no sense, e.g., when tunnelling
over a higher layer protocol such as IP. In that case the interface
has to go along (or before) the higher layer protocol is shutdown.

Kernel hhooks need to go last on teardown as they may be used at various
higher layers and we cannot remove them before we cleaned up the higher
layers.

For interface teardown there are multiple paths:
(a) a cloned interface is destroyed (inside a VIMAGE or in the base system),
(b) any interface is moved from a virtual network stack to a different
network stack ("vmove"), or (c) a virtual network stack is being shut down.
All code paths go through if_detach_internal() where we, depending on the
vmove flag or the vnet state, make a decision on how much to shut down;
in case we are destroying a VNET the individual protocol layers will
cleanup their own parts thus we cannot do so again for each interface as
we end up with, e.g., double-frees, destroying locks twice or acquiring
already destroyed locks.
When calling into protocol cleanups we equally have to tell them
whether they need to detach upper layer protocols ("ulp") or not
(e.g., in6_ifdetach()).

Provide or enahnce helper functions to do proper cleanup at a protocol
rather than at an interface level.

Approved by:		re (hrs)
Obtained from:		projects/vnet
Reviewed by:		gnn, jhb
Sponsored by:		The FreeBSD Foundation
MFC after:		2 weeks
Differential Revision:	https://reviews.freebsd.org/D6747
2016-06-21 13:48:49 +00:00
Kristof Provost
3e248e0fb4 pf: Filter on and set vlan PCP values
Adopt the OpenBSD syntax for setting and filtering on VLAN PCP values. This
introduces two new keywords: 'set prio' to set the PCP value, and 'prio' to
filter on it.

Reviewed by:    allanjude, araujo
Approved by:	re (gjb)
Obtained from:  OpenBSD (mostly)
Differential Revision:  https://reviews.freebsd.org/D6786
2016-06-17 18:21:55 +00:00
Kristof Provost
b599e8dc59 pf: Fix more ICMP mistranslation
In the default case fix the substitution of the destination address.

PR:		201519
Submitted by:	Max <maximos@als.nnov.ru>
MFC after:	1 week
2016-05-23 13:59:48 +00:00
Kristof Provost
c0c82715b8 pf: Fix ICMP translation
Fix ICMP source address rewriting in rdr scenarios.

PR:		201519
Submitted by:	Max <maximos@als.nnov.ru>
MFC after:	1 week
2016-05-23 12:41:29 +00:00
Kristof Provost
d9f4fce5a7 pf: Fix fragment timeout
We were inconsistent about the use of time_second vs. time_uptime.
Always use time_uptime so the value can be meaningfully compared.

Submitted by:	"Max" <maximos@als.nnov.ru>
MFC after:	4 days
2016-05-20 15:41:05 +00:00
Pedro F. Giffuni
a4641f4eaa sys/net*: minor spelling fixes.
No functional change.
2016-05-03 18:05:43 +00:00
Kristof Provost
0d8c93313e pf: Improve forwarding detection
When we guess the nature of the outbound packet (output vs. forwarding) we need
to take bridges into account. When bridging the input interface does not match
the output interface, but we're not forwarding. Similarly, it's possible for the
interface to actually be the bridge interface itself (and not a member interface).

PR:		202351
MFC after:	2 weeks
2016-03-16 06:42:15 +00:00
Kristof Provost
14b5e85b18 pf: Fix possible out-of-bounds write
In the DIOCRSETADDRS ioctl() handler we allocate a table for struct pfr_addrs,
which is processed in pfr_set_addrs(). At the users request we also provide
feedback on the deleted addresses, by storing them after the new list
('bcopy(&ad, addr + size + i, sizeof(ad));' in pfr_set_addrs()).

This means we write outside the bounds of the buffer we've just allocated.
We need to look at pfrio_size2 instead (i.e. the size the user reserved for our
feedback). That'd allow a malicious user to specify a smaller pfrio_size2 than
pfrio_size though, in which case we'd still read outside of the allocated
buffer. Instead we allocate the largest of the two values.

Reported By:	Paul J Murphy <paul@inetstat.net>
PR:		207463
MFC after:	5 days
Differential Revision:	https://reviews.freebsd.org/D5426
2016-02-25 07:33:59 +00:00