accel-ppp-users

[Accel-ppp-users] [PATCH 0/2] auth: fix negociation of authentication when requested by peer

[Guillaume N. <g....@al...>]

We have no way to authenticate ourself, we only know how to
authenticate the peer. Therefore we should reject any request made by
the peer to authenticate ourself. Failure to do so results in stalls
during the PPP authentication phase because accel-ppp never sends the
credentials the peer asked for.

Patch 1 rejects the Authentication-Protocol option sent by the peer and
expands on the reasons to do so.

Patch 2 removes a callback function that patch 1 made unused.

I must say that I can't really follow the logic of the original
auth_recv_conf_req() function. So it's always possible that I've missed
something. However, the principle of this series is that we really have
no way to authenticate ourself to the peer, so we should always reject
such attempts. This behaviour looks sane. We've run with a similar
patch for a few years and it helped us maximise compatibility with
broken clients.

Guillaume Nault (2):
  lcp: reject Authentication-Protocol option in Configure-Request
    packets
  auth: remove .recv_conf_req from struct ppp_auth_handler_t

 accel-pppd/auth/auth_chap_md5.c  |  8 -----
 accel-pppd/auth/auth_mschap_v1.c |  8 -----
 accel-pppd/auth/auth_mschap_v2.c |  8 -----
 accel-pppd/auth/auth_pap.c       |  7 -----
 accel-pppd/ppp/ppp_auth.c        | 53 +-------------------------------
 accel-pppd/ppp/ppp_auth.h        |  1 -
 6 files changed, 1 insertion(+), 84 deletions(-)

-- 
2.19.1

[Accel-ppp-users] [PATCH 2/2] auth: remove .recv_conf_req from struct ppp_auth_handler_t

[Guillaume N. <g....@al...>]

This callback isn't used anymore. Let's remove it from all
authentication backends.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/auth/auth_chap_md5.c  | 8 --------
 accel-pppd/auth/auth_mschap_v1.c | 8 --------
 accel-pppd/auth/auth_mschap_v2.c | 8 --------
 accel-pppd/auth/auth_pap.c       | 7 -------
 accel-pppd/ppp/ppp_auth.h        | 1 -
 5 files changed, 32 deletions(-)

diff --git a/accel-pppd/auth/auth_chap_md5.c b/accel-pppd/auth/auth_chap_md5.c
index ae062f53..de7f7418 100644
--- a/accel-pppd/auth/auth_chap_md5.c
+++ b/accel-pppd/auth/auth_chap_md5.c
@@ -185,13 +185,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 1;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	if (*ptr == CHAP_MD5)
-		return LCP_OPT_ACK;
-	return LCP_OPT_NAK;
-}
-
 static void chap_send_failure(struct chap_auth_data *ad)
 {
 	struct chap_failure msg = {
@@ -441,7 +434,6 @@ static struct ppp_auth_handler_t chap=
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = chap_start,
 	.finish        = chap_finish,
 	.check         = chap_check,
diff --git a/accel-pppd/auth/auth_mschap_v1.c b/accel-pppd/auth/auth_mschap_v1.c
index 67f941c6..23b0f0e9 100644
--- a/accel-pppd/auth/auth_mschap_v1.c
+++ b/accel-pppd/auth/auth_mschap_v1.c
@@ -186,13 +186,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 1;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	if (*ptr == MSCHAP_V1)
-		return LCP_OPT_ACK;
-	return LCP_OPT_NAK;
-}
-
 static void chap_send_failure(struct chap_auth_data *ad, char *mschap_error)
 {
 	struct chap_hdr *hdr = _malloc(sizeof(*hdr) + strlen(mschap_error) + 1);
@@ -514,7 +507,6 @@ static struct ppp_auth_handler_t chap = {
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = chap_start,
 	.finish        = chap_finish,
 	.check         = chap_check,
diff --git a/accel-pppd/auth/auth_mschap_v2.c b/accel-pppd/auth/auth_mschap_v2.c
index 5c82413c..66e17f24 100644
--- a/accel-pppd/auth/auth_mschap_v2.c
+++ b/accel-pppd/auth/auth_mschap_v2.c
@@ -187,13 +187,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 1;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	if (*ptr == MSCHAP_V2)
-		return LCP_OPT_ACK;
-	return LCP_OPT_NAK;
-}
-
 static void chap_send_failure(struct chap_auth_data *ad, char *mschap_error, char *reply_msg)
 {
 	struct chap_hdr *hdr = _malloc(sizeof(*hdr) + strlen(mschap_error) + strlen(reply_msg) + 4);
@@ -662,7 +655,6 @@ static struct ppp_auth_handler_t chap=
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = chap_start,
 	.finish        = chap_finish,
 	.check         = chap_check,
diff --git a/accel-pppd/auth/auth_pap.c b/accel-pppd/auth/auth_pap.c
index a290123e..a2303d12 100644
--- a/accel-pppd/auth/auth_pap.c
+++ b/accel-pppd/auth/auth_pap.c
@@ -29,7 +29,6 @@ static int conf_any_login = 0;
 static struct auth_data_t* auth_data_init(struct ppp_t *ppp);
 static void auth_data_free(struct ppp_t*, struct auth_data_t*);
 static int lcp_send_conf_req(struct ppp_t*, struct auth_data_t*, uint8_t*);
-static int lcp_recv_conf_req(struct ppp_t*, struct auth_data_t*, uint8_t*);
 static int pap_start(struct ppp_t*, struct auth_data_t*);
 static int pap_finish(struct ppp_t*, struct auth_data_t*);
 static void pap_recv(struct ppp_handler_t*h);
@@ -64,7 +63,6 @@ static struct ppp_auth_handler_t pap= {
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = pap_start,
 	.finish        = pap_finish,
 };
@@ -132,11 +130,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 0;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	return LCP_OPT_ACK;
-}
-
 static void pap_send_ack(struct pap_auth_data *p, int id)
 {
 	uint8_t buf[128];
diff --git a/accel-pppd/ppp/ppp_auth.h b/accel-pppd/ppp/ppp_auth.h
index c8df9fc6..e9398c29 100644
--- a/accel-pppd/ppp/ppp_auth.h
+++ b/accel-pppd/ppp/ppp_auth.h
@@ -22,7 +22,6 @@ struct ppp_auth_handler_t
 	const char *name;
 	struct auth_data_t* (*init)(struct ppp_t*);
 	int (*send_conf_req)(struct ppp_t*, struct auth_data_t*, uint8_t*);
-	int (*recv_conf_req)(struct ppp_t*, struct auth_data_t*, uint8_t*);
 	int (*start)(struct ppp_t*, struct auth_data_t*);
 	int (*finish)(struct ppp_t*, struct auth_data_t*);
 	void (*free)(struct ppp_t*,struct auth_data_t*);
-- 
2.19.1

[Accel-ppp-users] [PATCH 1/2] lcp: reject Authentication-Protocol option in Configure-Request packets

[Guillaume N. <g....@al...>]

If we receive a Configure-Request packet, that means the peer wants us
to authenticate to him. However, none of our authentication backends
(PAP, CHAP and MSCHAP v1/v2) supports authenticating ourself to the
peer. Therefore, the LCP negotiation completes, but we hang in the
authentication phase because accel-ppp never sends any credential.

We should reject the Authentication-Protocol option found in
Configure-Request packets sent by the peer. This way, the peer knows
that we won't authenticate to him. Then it's up to him to keep
connecting without authentication from our side or to drop the
connection.

This doesn't change the way we request the peer to authenticate to us.
That part of the negotiation is handled by Configure-Request packets
that are sent by us (not those sent by the peer).

In practice some PPP clients wouldn't connect with the previous
behaviour, but are perfectly happy with their Authentication-Protocol
option being rejected. They just resend their Configure-Request without
requesting authentication from our side.

Also, since the peer_auth field of struct auth_option_t is never set
anymore, we can remove the conditionals in auth_recv_conf_nak() and
auth_recv_conf_rej().

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/ppp/ppp_auth.c | 53 +--------------------------------------
 1 file changed, 1 insertion(+), 52 deletions(-)

diff --git a/accel-pppd/ppp/ppp_auth.c b/accel-pppd/ppp/ppp_auth.c
index 59248995..c475dadc 100644
--- a/accel-pppd/ppp/ppp_auth.c
+++ b/accel-pppd/ppp/ppp_auth.c
@@ -39,7 +39,6 @@ struct auth_option_t
 	struct lcp_option_t opt;
 	struct list_head auth_list;
 	struct auth_data_t *auth;
-	struct auth_data_t *peer_auth;
 	int started:1;
 };
 
@@ -142,57 +141,11 @@ static int auth_send_conf_req(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, u
 
 static int auth_recv_conf_req(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, uint8_t *ptr)
 {
-	struct auth_option_t *auth_opt = container_of(opt,typeof(*auth_opt),opt);
-	struct lcp_opt16_t *opt16 = (struct lcp_opt16_t*)ptr;
-	struct auth_data_t *d;
-	int r;
-
-	if (auth_opt->started) {
-		if (!auth_opt->auth)
-			return LCP_OPT_REJ;
-
-		if (!ptr || ntohs(opt16->val) != auth_opt->auth->proto)
-			return LCP_OPT_NAK;
-
-		return LCP_OPT_ACK;
-	}
-
-	if (list_empty(&auth_opt->auth_list))
-		return LCP_OPT_REJ;
-
-	if (!ptr)
-		return LCP_OPT_ACK;
-
-
-	list_for_each_entry(d, &auth_opt->auth_list, entry) {
-		if (d->proto == ntohs(opt16->val)) {
-			r = d->h->recv_conf_req(lcp->ppp, d, (uint8_t*)(opt16 + 1));
-			if (r == LCP_OPT_FAIL)
-				return LCP_OPT_FAIL;
-			if (r == LCP_OPT_REJ)
-				break;
-			auth_opt->peer_auth = d;
-			return r;
-		}
-	}
-
-	list_for_each_entry(d, &auth_opt->auth_list, entry) {
-		if (d->state != LCP_OPT_NAK) {
-			auth_opt->peer_auth = d;
-			return LCP_OPT_NAK;
-		}
-	}
-
-	log_ppp_error("cann't negotiate authentication type\n");
-	return LCP_OPT_FAIL;
+	return LCP_OPT_REJ;
 }
 
 static int auth_recv_conf_ack(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, uint8_t *ptr)
 {
-	struct auth_option_t *auth_opt = container_of(opt, typeof(*auth_opt), opt);
-
-	auth_opt->peer_auth = NULL;
-
 	return 0;
 }
 
@@ -206,8 +159,6 @@ static int auth_recv_conf_nak(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, u
 		return -1;
 	}
 	auth_opt->auth->state = LCP_OPT_NAK;
-	if (auth_opt->peer_auth)
-		auth_opt->auth = auth_opt->peer_auth;
 
 	list_for_each_entry(d, &auth_opt->auth_list, entry) {
 		if (d->state != LCP_OPT_NAK)
@@ -229,8 +180,6 @@ static int auth_recv_conf_rej(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, u
 	}
 
 	auth_opt->auth->state = LCP_OPT_NAK;
-	if (auth_opt->peer_auth)
-		auth_opt->auth = auth_opt->peer_auth;
 
 	list_for_each_entry(d, &auth_opt->auth_list, entry) {
 		if (d->state != LCP_OPT_NAK)
-- 
2.19.1

Re: [Accel-ppp-users] [PATCH 0/2] auth: fix negociation of authentication when requested by peer

[Dmitry K. <xe...@ma...>]

>We have no way to authenticate ourself, we only know how to
>authenticate the peer. Therefore we should reject any request made by
>the peer to authenticate ourself. Failure to do so results in stalls
>during the PPP authentication phase because accel-ppp never sends the
>credentials the peer asked for.
>
>Patch 1 rejects the Authentication-Protocol option sent by the peer and
>expands on the reasons to do so.
>
>Patch 2 removes a callback function that patch 1 made unused.
>
>I must say that I can't really follow the logic of the original
>auth_recv_conf_req() function. So it's always possible that I've missed
>something. However, the principle of this series is that we really have
>no way to authenticate ourself to the peer, so we should always reject
>such attempts. This behaviour looks sane. We've run with a similar
>patch for a few years and it helped us maximise compatibility with
>broken clients.
>
>Guillaume Nault (2):
>  lcp: reject Authentication-Protocol option in Configure-Request
>    packets
>  auth: remove .recv_conf_req from struct ppp_auth_handler_t

Applied, thanks

-- 
Dmitry Kozlov