Merge ~epics-core/epics-base/+git/asLib:as-hostname into ~epics-core/epics-base/+git/epics-base:7.0

[mdavidsaver]

Lets give CA Access Security some teeth. At least as far as is possible w/o protocol changes.

Add a flag which replaces use of CA client provided host name strings with the peer IP address, which isn't so trivially forged. This is coupled with a change to asLib to cause a reverse lookup of hostnames found in ACF files. So the "names" stored as all IP addresses. This is done when an ACF file load, so subsequent DNS changes wouldn't be picked up until/unless the ACF file is reloaded.

As a first attempt, the switch to change behavior is a global variable 'asUseIP'. This was quick to implement as I expect ANJ will have other ideas.

[Andrew Johnson (anj)]

Calls for a parallel change to PCAS, once this has been merged.

Keep with a variable for now, consider adding an environment variable for default setting once we've had some experience with using this.

Test what happens if you switch it on/off at runtime.

Documentation – update the chapter of the AppDevGuide, as well as a Release Notes entry ("experimental"?).

[mdavidsaver (mdavidsaver)]

I've pushed a change which addresses reviewer comments. A fresh clone pull it down, but the LP web frontend doesn't show it in the as-hostname branch history. The commit can be directly referenced. Not sure what is going on...

https://git.launchpad.net/~epics-core/epics-base/+git/asLib/commit/?id=73cdea5517d625243ba149abf4a1368fbae8fe81

https://github.com/mdavidsaver/epics-base/commit/73cdea5517d625243ba149abf4a1368fbae8fe81

[Andrew Johnson (anj)]

Weird, looks like a Launchpad bug. I just changed Launchpad's "default branch" setting for that repo to be refs/heads/as-hostname although doing that didn't trigger any updates. It was set to refs/heads/master but that branch doesn't actually exist, which might be why LP got confused. I'm about to push a change expanding the Release Notes entry in the hopes that a commit to the repo's default branch might trigger Launchpad to wake up; we'll see whether it does shortly.

Question: I think we said that if a host-name cannot be found, this will cause the IOC to start up with all access disabled. Is this the behavior that we actually want? Wouldn't it be better to allow other hosts that can be resolved to still connect? I suspect our operational ASC files have any number of no-longer-valid host names in them, and here the act of removing a long-disused host name from our DNS is something that could our IT group might do at any time, so it shouldn't prevent our control system from starting up (frankly that would be an operational nightmare). If you change this behavior you should also adjust the Release Note to match (new consequence #2).

[mdavidsaver (mdavidsaver)]

> if a host-name cannot be found, this will cause the IOC to start up with all access disabled. Is this the behavior that we actually want?

That's a fair point. As I think about it, there is no reason to fail the whole load if one host doesn't lookup. It would be easy enough to fill in "unresolved" instead of an IP, print a warning, and continue.

(personally, I'd like to eliminate read access restrictions all together)

> something that could our IT group might do at any time

To some extent, this could be mitigated by including both hostname and IP for some important hosts. eg, hosts which can trigger ACF reload.

Ultimately though, turning this feature on implies an acceptance of this risk.

review: Needs Fixing

[mdavidsaver (mdavidsaver)]

Name resolution now fails soft.

[Andrew Johnson (anj)]

Since you changed it to fail soft, the release note needs updating to match. The wording of the second list bullet item is now wrong, and the end of the preceding text paragraph might need tweaking as well, not sure about that.

[mdavidsaver (mdavidsaver)]

Release notes updated.

review: Approve

[Andrew Johnson (anj)]

AI on AJ to review soon.

[Bruce Hill (bhill)]

Successfully tested this patch w/ pvAccess gwdev branch and pva2pva d7314ea from mdavidsaver.

Tested IOC and ca-gateway instances w/ asCheckClientIP=0 and asCheckClientIP=1.
ca-gateway was based on R2-1-1-0 with inline code to set asCheckClientIP.

No changes were needed to pcas version 4.13.2.

With asCheckClientIP=0, a hacked caput can bypass ASG RULES using hostnames.
With asCheckClientIP=1, caput can only spoof username

For pvput and IOC testing:
With asCheckClientIP=0, All ASG RULES using HAG deny write access.
With asCheckClientIP=1, pvput works same as CA for all UAG and HAG based ASG RULES. (Didn't test variables in RULES or spoofing username in pvput.)

Also tested w/ new p4p gateway as a client while p4p gateway is running it's own access security.

[Bruce Hill (bhill)]

Re the initial default for asCheckClientIP, I recommend it be set to 1.

Defaulting to 0 only helps those who run w/o DNS yet also want to run EPICS 7 IOCs
but continue to use CA instead of PVA to modify their access secured PVs.
They also have an easy workaround by using IP addr in their acf files.

Meanwhile, every site who tries to support access security w/ EPICS 7 could easily
miss the need to set asCheckClientIP in order to make pvAccess support HAG based rules.

It also has the benefit of closing the CA spoofed hostname vulnerability.

[Ernest Williams (ernesto-slac)]

I agree

Sent from my iPhone

> On Aug 13, 2019, at 11:10 PM, Bruce Hill via Core-talk <email address hidden> wrote:
>
> Re the initial default for asCheckClientIP, I recommend it be set to 1.
>
> Defaulting to 0 only helps those who run w/o DNS yet also want to run EPICS 7 IOCs
> but continue to use CA instead of PVA to modify their access secured PVs.
> They also have an easy workaround by using IP addr in their acf files.
>
> Meanwhile, every site who tries to support access security w/ EPICS 7 could easily
> miss the need to set asCheckClientIP in order to make pvAccess support HAG based rules.
>
> It also has the benefit of closing the CA spoofed hostname vulnerability.
> --
> https://code.launchpad.net/~epics-core/epics-base/+git/asLib/+merge/358822
> Your team EPICS Core Developers is subscribed to branch epics-base:7.0.

[Andrew Johnson (anj)]

Minor Q: An IPv4 address represented as an ASCII dotted-quad needs no more than 16 chars including the string terminator. Why are you (consistently) allocating 24? With a port number it would need 22 chars, and 24 chars would be too short for an IPv6 address. Just wondering...

On the default value of asCheckClientIP, my concern with making it 1 is that this could change the behavior for some existing working systems. If the name for a client listed in an IOC's ASCF is the canonical hostname for the client machine but the network path to that IOC comes through a secondary network interface with a different DNS name, a previously working ASCF rule will now deny access to that client. Admittedly this is an unusual situation but it's by no means impossible, and the code currently has no way to detect when it happens (it could, but Michael has better things to do with his time than implement that). If we do change the default the release notes would need changing to match, and they should probably have something in bold or red to get people to notice.

Approved.

review: Approve

[mdavidsaver (mdavidsaver)]

I'm planning to go ahead and merge with a default of asCheckClientIP=0. Changing this can be a separate discussion.

> Why are you (consistently) allocating 24?

Force of habit? I can't recall.

> default value of asCheckClientIP

Can we agree that the desired final solution is defaulting to asCheckClientIP=1 ?

> Admittedly this is an unusual situation

Yup, given the number of sites not using ACF at all.

Is it worth my arguing that any site bothering with access control
would likely welcome stronger authentication?

> canonical hostname ... a secondary network interface with a different DNS name
> ... it could ...

How could this be done? I can't think of a way.

> has better things to do

Granted. It would still be nice to document a possible course of action in case someone is motivated.

[Andrew Johnson (anj)]

> Can we agree that the desired final solution is defaulting to asCheckClientIP=1 ?

Yes, I'd just like there to be something like a couple of releases where people can try it out before we flip the default.

> Is it worth my arguing that any site bothering with access control would likely welcome stronger authentication?

We use AS here to prevent people from fiddling with PVs that only the experts should be modifying, such as some of our RF system IOCs where a mis-placed put might be able to destroy one of our dwindling supply of RF cavities. I'm totally guessing whether that might be possible, but I do know that our RF IOCs do use it quite extensively, as do our beamline gateways. I agree that stronger authentication is better, and that this will give us stronger authentication.

> How could this be done?

Instead of making this a binary switch, store both the HAG name and the DNS result. Implement the host comparisons using both methods simultaneously and cross-check the results. When the by-name comparison says Match but the by-DNS says No there's either someone trying to impersonate a different client machine or this could be a multiple-interface name issue. You aren't modifying the actual host comparison code at all here though, so doing this would need more extensive changes to asLib.

Similarly larger changes would be needed to allow the use of IP address ranges in the ASCF (e.g. 192.168.12.16/28 say) which I could see as being useful to some (and would need more than 16 chars to store).

diff --git a/documentation/RELEASE_NOTES.html b/documentation/RELEASE_NOTES.html
index df78360..39d101b 100644
--- a/documentation/RELEASE_NOTES.html
+++ b/documentation/RELEASE_NOTES.html
@@ -30,6 +30,7 @@ release.</p>
 
 -->
 
+<<<<<<< documentation/RELEASE_NOTES.html
 <h1 align="center">EPICS Release 7.0.2.2</h1>
 
 <h3>Build System changes</h3>
@@ -127,6 +128,50 @@ than it used to be.</p>
 
 
 <h1 align="center">EPICS Release 7.0.2</h1>
+=======
+<h3>Channel Access Security: Check Hostname Against DNS</h3>
+
+<p>Host names given in a <tt>HAG</tt> entry of an IOC's Access Security
+Configuration File (ACF) have to date been compared against the hostname
+provided by the CA client at connection time, which may or may not be the actual
+name of that client. This allows rogue clients to pretend to be a different
+host, and the IOC would believe them.</p>
+
+<p>An option is now available to cause an IOC to ask its operating system to
+look up the IP address of any hostnames listed in its ACF (which will normally
+be done using the DNS or the <tt>/etc/hosts</tt> file). The IOC will then
+compare the resulting IP address against the client's actual IP address when
+checking access permissions at connection time. This name resolution is performed
+at ACF file load time, which has a few consequences:</p>
+
+<ol>
+
+<li>If the DNS is slow when the names are resolved this will delay the process
+of loading the ACF file.</li>
+
+<li>If a host name cannot be resolved the IOC will proceed, but this host name
+will never be matched.</li>
+
+<li>Any changes in the hostname to IP address mapping will not be picked up by
+the IOC unless and until the ACF file gets reloaded.</li>
+
+</ol>
+
+<p>Optionally, IP addresses may be added instead of, or in addition to, host names
+in the ACF file.</p>
+
+<p>This feature can be enabled before <tt>iocInit</tt> with</p>
+
+<blockquote><pre>
+var("asCheckClientIP",1)
+</pre></blockquote>
+
+<p>or with the VxWorks target shell use</p>
+
+<blockquote><pre>
+asCheckClientIP = 1
+</pre></blockquote>
+>>>>>>> documentation/RELEASE_NOTES.html
 
 <h3>Launchpad Bugs</h3>
 
diff --git a/modules/database/src/ioc/rsrv/camessage.c b/modules/database/src/ioc/rsrv/camessage.c
index 72a4b17..f54bb48 100644
--- a/modules/database/src/ioc/rsrv/camessage.c
+++ b/modules/database/src/ioc/rsrv/camessage.c
@@ -861,6 +861,14 @@ static int host_name_action ( caHdrLargeArray *mp, void *pPayload,
         return RSRV_ERROR;
     }
 
+    /* after all validation */
+    if(asCheckClientIP) {
+
+        DLOG (2, ( "CAS: host_name_action for \"%s\" ignores client provided host name\n",
+            client->pHostName ) );
+        return RSRV_OK;
+    }
+
     /*
      * user name will not change if there isnt enough memory
      */
diff --git a/modules/database/src/ioc/rsrv/caservertask.c b/modules/database/src/ioc/rsrv/caservertask.c
index f377d83..7a9ae63 100644
--- a/modules/database/src/ioc/rsrv/caservertask.c
+++ b/modules/database/src/ioc/rsrv/caservertask.c
@@ -1421,6 +1421,20 @@ struct client *create_tcp_client (SOCKET sock , const osiSockAddr *peerAddr)
     }
 
     client->addr = peerAddr->ia;
+    if(asCheckClientIP) {
+        epicsUInt32 ip = ntohl(client->addr.sin_addr.s_addr);
+        client->pHostName = malloc(24);
+        if(!client->pHostName) {
+            destroy_client ( client );
+            return NULL;
+        }
+        epicsSnprintf(client->pHostName, 24,
+                      "%u.%u.%u.%u",
+                      (ip>>24)&0xff,
+                      (ip>>16)&0xff,
+                      (ip>>8)&0xff,
+                      (ip>>0)&0xff);
+    }
 
     /*
      * see TCP(4P) this seems to make unsolicited single events much
diff --git a/modules/database/src/ioc/rsrv/server.h b/modules/database/src/ioc/rsrv/server.h
index 4d502f7..6392c69 100644
--- a/modules/database/src/ioc/rsrv/server.h
+++ b/modules/database/src/ioc/rsrv/server.h
@@ -86,7 +86,7 @@ typedef struct client {
   ELLLIST               chanList;
   ELLLIST               chanPendingUpdateARList;
   ELLLIST               putNotifyQue;
-  struct sockaddr_in    addr;
+  struct sockaddr_in    addr; /* peer address, TCP only */
   epicsTimeStamp        time_at_last_send;
   epicsTimeStamp        time_at_last_recv;
   void                  *evuser;
diff --git a/modules/libcom/src/as/asLib.h b/modules/libcom/src/as/asLib.h
index 261e5ed..528ce6e 100644
--- a/modules/libcom/src/as/asLib.h
+++ b/modules/libcom/src/as/asLib.h
@@ -21,6 +21,11 @@
 extern "C" {
 #endif
 
+/* 0 - Use (unverified) client provided host name string.
+ * 1 - Use actual client IP address.  HAG() are resolved to IPs at ACF load time.
+ */
+epicsShareExtern int asCheckClientIP;
+
 typedef struct asgMember *ASMEMBERPVT;
 typedef struct asgClient *ASCLIENTPVT;
 typedef int (*ASINPUTFUNCPTR)(char *buf,int max_size);
@@ -165,8 +170,8 @@ typedef struct uag{
 } UAG;
 /*Defs for Host Access Groups*/
 typedef struct{
-	ELLNODE	node;
-	char	*host;
+    ELLNODE	node;
+    char	host[1];
 } HAGNAME;
 typedef struct hag{
 	ELLNODE	node;
diff --git a/modules/libcom/src/as/asLibRoutines.c b/modules/libcom/src/as/asLibRoutines.c
index 3f5713e..ab0bf50 100644
--- a/modules/libcom/src/as/asLibRoutines.c
+++ b/modules/libcom/src/as/asLibRoutines.c
@@ -15,6 +15,8 @@
 #include <ctype.h>
 
 #define epicsExportSharedSymbols
+#include "osiSock.h"
+#include "epicsTypes.h"
 #include "epicsStdio.h"
 #include "dbDefs.h"
 #include "epicsThread.h"
@@ -27,6 +29,8 @@
 #include "postfix.h"
 #include "asLib.h"
 
+int asCheckClientIP;
+
 static epicsMutexId asLock;
 #define LOCK epicsMutexMustLock(asLock)
 #define UNLOCK epicsMutexUnlock(asLock)
@@ -1203,14 +1207,38 @@ static HAG *asHagAdd(const char *hagName)
 static long asHagAddHost(HAG *phag,const char *host)
 {
     HAGNAME *phagname;
-    int     len, i;
 
     if (!phag) return 0;
-    len = strlen(host);
-    phagname = asCalloc(1, sizeof(HAGNAME) + len + 1);
-    phagname->host = (char *)(phagname + 1);
-    for (i = 0; i < len; i++) {
-        phagname->host[i] = (char)tolower((int)host[i]);
+    if(!asCheckClientIP) {
+        size_t i, len = strlen(host);
+        phagname = asCalloc(1, sizeof(HAGNAME) + len);
+        for (i = 0; i < len; i++) {
+            phagname->host[i] = (char)tolower((int)host[i]);
+        }
+
+    } else {
+        struct sockaddr_in addr;
+        epicsUInt32 ip;
+
+        if(aToIPAddr(host, 0, &addr)) {
+            static const char unresolved[] = "unresolved:";
+
+            errlogPrintf("ACF: Unable to resolve host '%s'\n", host);
+
+            phagname = asCalloc(1, sizeof(HAGNAME) + sizeof(unresolved)-1+strlen(host));
+            strcpy(phagname->host, unresolved);
+            strcat(phagname->host, host);
+
+        } else {
+            ip = ntohl(addr.sin_addr.s_addr);
+            phagname = asCalloc(1, sizeof(HAGNAME) + 24);
+            epicsSnprintf(phagname->host, 24,
+                          "%u.%u.%u.%u",
+                          (ip>>24)&0xff,
+                          (ip>>16)&0xff,
+                          (ip>>8)&0xff,
+                          (ip>>0)&0xff);
+        }
     }
     ellAdd(&phag->list, &phagname->node);
     return 0;
diff --git a/modules/libcom/src/iocsh/libComRegister.c b/modules/libcom/src/iocsh/libComRegister.c
index b105ea1..8fd5e79 100644
--- a/modules/libcom/src/iocsh/libComRegister.c
+++ b/modules/libcom/src/iocsh/libComRegister.c
@@ -12,6 +12,7 @@
 
 #define epicsExportSharedSymbols
 #include "iocsh.h"
+#include "asLib.h"
 #include "epicsStdioRedirect.h"
 #include "epicsString.h"
 #include "epicsTime.h"
@@ -393,6 +394,8 @@ static void installLastResortEventProviderCallFunc(const iocshArgBuf *args)
     installLastResortEventProvider();
 }
 
+static iocshVarDef asCheckClientIPDef = {"asCheckClientIP", iocshArgInt, 0};
+
 void epicsShareAPI libComRegister(void)
 {
     iocshRegister(&dateFuncDef, dateCallFunc);
@@ -425,4 +428,7 @@ void epicsShareAPI libComRegister(void)
     
     iocshRegister(&generalTimeReportFuncDef,generalTimeReportCallFunc);
     iocshRegister(&installLastResortEventProviderFuncDef, installLastResortEventProviderCallFunc);
+
+    asCheckClientIPDef.pval = &asCheckClientIP;
+    iocshRegisterVariable(&asCheckClientIPDef);
 }
diff --git a/modules/libcom/test/aslibtest.c b/modules/libcom/test/aslibtest.c
index 875aa56..4237faf 100644
--- a/modules/libcom/test/aslibtest.c
+++ b/modules/libcom/test/aslibtest.c
@@ -81,8 +81,8 @@ static const char hostname_config[] = ""
 
 static void testHostNames(void)
 {
-
     testDiag("testHostNames()");
+    asCheckClientIP = 0;
 
     testOk1(asInitMem(hostname_config, NULL)==0);
 
@@ -102,18 +102,53 @@ static void testHostNames(void)
     testAccess("ro", 0);
     testAccess("rw", 0);
 
-    setHost("nosuchhost");
+    setHost("guaranteed.invalid.");
+
+    testAccess("invalid", 0);
+    testAccess("DEFAULT", 0);
+    testAccess("ro", 0);
+    testAccess("rw", 0);
+}
+
+static void testUseIP(void)
+{
+    testDiag("testUseIP()");
+    asCheckClientIP = 1;
+
+    /* still host names in .acf */
+    testOk1(asInitMem(hostname_config, NULL)==0);
+    /* now resolved to IPs */
+
+    setUser("testing");
+    setHost("localhost"); /* will not match against resolved IP */
+    asAsl = 0;
+
+    testAccess("invalid", 0);
+    testAccess("DEFAULT", 0);
+    testAccess("ro", 0);
+    testAccess("rw", 0);
+
+    setHost("127.0.0.1");
+
+    testAccess("invalid", 0);
+    testAccess("DEFAULT", 0);
+    testAccess("ro", 1);
+    testAccess("rw", 3);
+
+    setHost("guaranteed.invalid.");
 
     testAccess("invalid", 0);
     testAccess("DEFAULT", 0);
     testAccess("ro", 0);
     testAccess("rw", 0);
 }
+
 MAIN(aslibtest)
 {
-    testPlan(14);
+    testPlan(27);
     testSyntaxErrors();
     testHostNames();
+    testUseIP();
     errlogFlush();
     return testDone();
 }