| Message ID | 20210222175500.6189-2-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | icmp: randomize the global rate limiter | expand |
On Mon, Feb 22, 2021 at 10:55:00AM -0700, Tim Gardner wrote: > From: Eric Dumazet <edumazet@google.com> > > BugLink: https://bugs.launchpad.net/bugs/1902115 > > [ Upstream commit b38e7819cae946e2edf869e604af1e65a5d241c5 ] > > Keyu Man reported that the ICMP rate limiter could be used > by attackers to get useful signal. Details will be provided > in an upcoming academic publication. > > Our solution is to add some noise, so that the attackers > no longer can get help from the predictable token bucket limiter. > > Fixes: 4cdf507d5452 ("icmp: add a global rate limitation") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Keyu Man <kman001@ucr.edu> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Kamal Mostafa <kamal@canonical.com> > Signed-off-by: Ian May <ian.may@canonical.com> > (cherry picked from focal/oracle-5.4 commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3) > CVE-2020-25705 > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > > back port notes: This was a clean cherry pick of the Focal oracle-5.4 backport. As for the fix for CVE-2020-25668, I find it unusual to document the backport like this. I do take backports from different series sometimes, making sure they make sense for the different series I am applying them to. However, the line I care about is present here and this is better documenting the source of the backport than when only the code is taken from the different series. Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- > Documentation/networking/ip-sysctl.txt | 4 +++- > net/ipv4/icmp.c | 7 +++++-- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt > index 5f53faff4e25..151d6ad0e0a7 100644 > --- a/Documentation/networking/ip-sysctl.txt > +++ b/Documentation/networking/ip-sysctl.txt > @@ -1005,12 +1005,14 @@ icmp_ratelimit - INTEGER > icmp_msgs_per_sec - INTEGER > Limit maximal number of ICMP packets sent per second from this host. > Only messages whose type matches icmp_ratemask (see below) are > - controlled by this limit. > + controlled by this limit. For security reasons, the precise count > + of messages per second is randomized. > Default: 1000 > > icmp_msgs_burst - INTEGER > icmp_msgs_per_sec controls number of ICMP packets sent per second, > while icmp_msgs_burst controls the burst size of these packets. > + For security reasons, the precise burst size is randomized. > Default: 50 > > icmp_ratemask - INTEGER > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index f369e7ce685b..dcffda472585 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -239,7 +239,7 @@ static struct { > /** > * icmp_global_allow - Are we allowed to send one more ICMP message ? > * > - * Uses a token bucket to limit our ICMP messages to sysctl_icmp_msgs_per_sec. > + * Uses a token bucket to limit our ICMP messages to ~sysctl_icmp_msgs_per_sec. > * Returns false if we reached the limit and can not send another packet. > * Note: called with BH disabled > */ > @@ -267,7 +267,10 @@ bool icmp_global_allow(void) > } > credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst); > if (credit) { > - credit--; > + /* We want to use a credit of one in average, but need to randomize > + * it for security reasons. > + */ > + credit = max_t(int, credit - prandom_u32_max(3), 0); > rc = true; > } > WRITE_ONCE(icmp_global.credit, credit); > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 22.02.21 22:07, Thadeu Lima de Souza Cascardo wrote: > On Mon, Feb 22, 2021 at 10:55:00AM -0700, Tim Gardner wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> BugLink: https://bugs.launchpad.net/bugs/1902115 >> >> [ Upstream commit b38e7819cae946e2edf869e604af1e65a5d241c5 ] >> >> Keyu Man reported that the ICMP rate limiter could be used >> by attackers to get useful signal. Details will be provided >> in an upcoming academic publication. >> >> Our solution is to add some noise, so that the attackers >> no longer can get help from the predictable token bucket limiter. >> >> Fixes: 4cdf507d5452 ("icmp: add a global rate limitation") >> Signed-off-by: Eric Dumazet <edumazet@google.com> >> Reported-by: Keyu Man <kman001@ucr.edu> >> Signed-off-by: Jakub Kicinski <kuba@kernel.org> >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Signed-off-by: Kamal Mostafa <kamal@canonical.com> >> Signed-off-by: Ian May <ian.may@canonical.com> >> (cherry picked from focal/oracle-5.4 commit fc3d57b1e860f9bdd97bbbf14f1617e718fae7f3) >> CVE-2020-25705 >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> >> back port notes: This was a clean cherry pick of the Focal oracle-5.4 backport. When bringing up backport notes I was saying that we document what we did *if* we do backports. And I gave an exact example of how this is done by everybody else. But regardless of that, once a thread is NACKed once it is dead. Re-submissions go into a new thread. Having one thread with a mix of ok and rejected patches mixed is just too dangerous. -Stefan > > As for the fix for CVE-2020-25668, I find it unusual to document the backport > like this. I do take backports from different series sometimes, making sure > they make sense for the different series I am applying them to. > > However, the line I care about is present here and this is better documenting > the source of the backport than when only the code is taken from the different > series. > > Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > >> --- >> Documentation/networking/ip-sysctl.txt | 4 +++- >> net/ipv4/icmp.c | 7 +++++-- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt >> index 5f53faff4e25..151d6ad0e0a7 100644 >> --- a/Documentation/networking/ip-sysctl.txt >> +++ b/Documentation/networking/ip-sysctl.txt >> @@ -1005,12 +1005,14 @@ icmp_ratelimit - INTEGER >> icmp_msgs_per_sec - INTEGER >> Limit maximal number of ICMP packets sent per second from this host. >> Only messages whose type matches icmp_ratemask (see below) are >> - controlled by this limit. >> + controlled by this limit. For security reasons, the precise count >> + of messages per second is randomized. >> Default: 1000 >> >> icmp_msgs_burst - INTEGER >> icmp_msgs_per_sec controls number of ICMP packets sent per second, >> while icmp_msgs_burst controls the burst size of these packets. >> + For security reasons, the precise burst size is randomized. >> Default: 50 >> >> icmp_ratemask - INTEGER >> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c >> index f369e7ce685b..dcffda472585 100644 >> --- a/net/ipv4/icmp.c >> +++ b/net/ipv4/icmp.c >> @@ -239,7 +239,7 @@ static struct { >> /** >> * icmp_global_allow - Are we allowed to send one more ICMP message ? >> * >> - * Uses a token bucket to limit our ICMP messages to sysctl_icmp_msgs_per_sec. >> + * Uses a token bucket to limit our ICMP messages to ~sysctl_icmp_msgs_per_sec. >> * Returns false if we reached the limit and can not send another packet. >> * Note: called with BH disabled >> */ >> @@ -267,7 +267,10 @@ bool icmp_global_allow(void) >> } >> credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst); >> if (credit) { >> - credit--; >> + /* We want to use a credit of one in average, but need to randomize >> + * it for security reasons. >> + */ >> + credit = max_t(int, credit - prandom_u32_max(3), 0); >> rc = true; >> } >> WRITE_ONCE(icmp_global.credit, credit); >> -- >> 2.17.1 >> >> >> -- >> kernel-team mailing list >> kernel-team@lists.ubuntu.com >> https://lists.ubuntu.com/mailman/listinfo/kernel-team >
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt index 5f53faff4e25..151d6ad0e0a7 100644 --- a/Documentation/networking/ip-sysctl.txt +++ b/Documentation/networking/ip-sysctl.txt @@ -1005,12 +1005,14 @@ icmp_ratelimit - INTEGER icmp_msgs_per_sec - INTEGER Limit maximal number of ICMP packets sent per second from this host. Only messages whose type matches icmp_ratemask (see below) are - controlled by this limit. + controlled by this limit. For security reasons, the precise count + of messages per second is randomized. Default: 1000 icmp_msgs_burst - INTEGER icmp_msgs_per_sec controls number of ICMP packets sent per second, while icmp_msgs_burst controls the burst size of these packets. + For security reasons, the precise burst size is randomized. Default: 50 icmp_ratemask - INTEGER diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index f369e7ce685b..dcffda472585 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -239,7 +239,7 @@ static struct { /** * icmp_global_allow - Are we allowed to send one more ICMP message ? * - * Uses a token bucket to limit our ICMP messages to sysctl_icmp_msgs_per_sec. + * Uses a token bucket to limit our ICMP messages to ~sysctl_icmp_msgs_per_sec. * Returns false if we reached the limit and can not send another packet. * Note: called with BH disabled */ @@ -267,7 +267,10 @@ bool icmp_global_allow(void) } credit = min_t(u32, icmp_global.credit + incr, sysctl_icmp_msgs_burst); if (credit) { - credit--; + /* We want to use a credit of one in average, but need to randomize + * it for security reasons. + */ + credit = max_t(int, credit - prandom_u32_max(3), 0); rc = true; } WRITE_ONCE(icmp_global.credit, credit);