GSMA-CPAS / BWRP-chaincode

Apache License 2.0
1 stars 0 forks source link

Possible to control input variable by caller #2

Closed Horizon-Developer closed 4 years ago

Horizon-Developer commented 4 years ago

https://github.com/GSMA-CPAS/BWRP-chaincode/blob/0e87ab7f56e564e12ac12a0e8cd39442f6ca60d2/hybrid/offchain.go#L313

The TargetMSP is under control of the invoking party. Changing this to the LOCAL_PEER_ID would be advisable.

sschulz-t commented 4 years ago

Thanks for spotting this!

Let's say ORG1 wants to share a document with ORG2.

1) It calls @peer_org1 StorePrivateDocument(..., ORG2, ...) 2) It calls @peer_org2 StorePrivateDocument(..., ORG2, ...)

We need this identifier in order to store both interacting parties. When we use os.Getenv("CORE_PEER_LOCALMSPID") we loose that functionallity.

I am thinking of how you can abuse this. the calling MSP is always stored in the fromMSP field. So the origin is traceable. If I store targetMSP=ORG3 on the database of ORG2 all that happens is that I created invalid data.

We should be able to detect this invalid data and prevent this to happen. A possible workaround might be to allow only local (=trusted) overrides:

// only allow target override if called locally
localMSPID := os.Getenv("CORE_PEER_LOCALMSPID")
if (invokingMSPID != localMSPID){
  // called from a external MSP
  if (targetMSPID != localMSPID){
    // external MSP wants to set an invalid targetMSP
    return "", fmt.Errorf("forbidden: invalid targetMSPID")
  }
}
Horizon-Developer commented 4 years ago

Thanks for spotting this!

Let's say ORG1 wants to share a document with ORG2.

  1. It calls @peer_org1 StorePrivateDocument(..., ORG2, ...)
  2. It calls @peer_org2 StorePrivateDocument(..., ORG2, ...)

We need this identifier in order to store both interacting parties. When we use os.Getenv("CORE_PEER_LOCALMSPID") we loose that functionallity.

I am thinking of how you can abuse this. the calling MSP is always stored in the fromMSP field. So the origin is traceable. If I store targetMSP=ORG3 on the database of ORG2 all that happens is that I created invalid data.

We should be able to detect this invalid data and prevent this to happen. A possible workaround might be to allow only local (=trusted) overrides:

// only allow target override if called locally
localMSPID := os.Getenv("CORE_PEER_LOCALMSPID")
if (invokingMSPID != localMSPID){
  // called from a external MSP
  if (targetMSPID != localMSPID){
    // external MSP wants to set an invalid targetMSP
    return "", fmt.Errorf("forbidden: invalid targetMSPID")
  }
}

Yes totally agree with the proposed solution. I will keep this issue open so you can commit the code referencing this issue, and then when it is in main we can close it.