Skip to content

Commit

Permalink
Safely ignore marker, UAT, unsupported, malformed packets (closes hoc…
Browse files Browse the repository at this point in the history 
…keypuck#135, closes hockeypuck#198)
  • Loading branch information
@andrewgdotcom
andrewgdotcom committed Dec 4, 2023
1 parent 5425d0e commit c61a146
Show file tree
Hide file tree
Showing 21 changed files with 77 additions and 730 deletions.
4 changes: 0 additions & 4 deletions contrib/templates/index.html.tmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@
{{ range $sig := $uid.Signatures }}sig {{ if $sig.Revocation }}<span class="warn">revok </span>{{ else }} sig {{ end }}<a href="/pks/lookup?op=get&search=0x{{ $sig.IssuerKeyID }}">{{ $sig.IssuerKeyID }}</a> {{ $sig.Creation }} {{ if $sig.Expiration }}{{ $sig.Expiration }}{{ else }}{{ $spacer }}{{ end }} {{ $spacer }} <a href="/pks/lookup?op=vindex&search=0x{{ $sig.IssuerKeyID }}">{{ if eq $sig.IssuerKeyID $key.LongKeyID }}[selfsig]{{ else }}{{ $sig.IssuerKeyID }}{{ end }}</a>
{{ end }}
{{ end -}}
{{ range $uat := $key.UserAttrs }}<strong>uat</strong> {{ range $photo := $uat.Photos }}<img src="{{ url $photo.DataURI }}">{{end}}
{{ range $sig := $uat.Signatures }}sig {{ if $sig.Revocation }}<span class="warn">revok </span>{{ else }} sig {{ end }}<a href="/pks/lookup?op=get&search=0x{{ $sig.IssuerKeyID }}">{{ $sig.IssuerKeyID }}</a> {{ $sig.Creation }} {{ if $sig.Expiration }}{{ $sig.Expiration }}{{ else }}{{ $spacer }}{{ end }} {{ $spacer }} <a href="/pks/lookup?op=vindex&search=0x{{ $sig.IssuerKeyID }}">{{ if eq $sig.IssuerKeyID $key.LongKeyID }}[selfsig]{{ else }}{{ $sig.IssuerKeyID }}{{ end }}</a>
{{ end }}
{{ end -}}
{{ range $sub := $key.SubKeys }}<strong>sub</strong> ({{ $sub.Version }}){{ $sub.Algorithm.Name }}{{ $sub.BitLength }}/{{ if $fp }}{{ $sub.Fingerprint }}{{ else }}{{ $sub.LongKeyID }}{{ end }} {{ $sub.Creation }}
{{ range $sig := $sub.Signatures }}sig {{ if $sig.Revocation }}<span class="warn">revok </span>{{ else }}sbind{{ end }} <a href="/pks/lookup?op=get&search=0x{{ $key.LongKeyID }}">{{ $key.LongKeyID }}</a> {{ $sig.Creation }} {{ $spacer }} {{ if $sig.Expiration }}{{ $sig.Expiration }}{{ else }}{{ $spacer }}{{ end }} <a href="/pks/lookup?op=vindex&search=0x{{ $key.LongKeyID }}">[]</a>
{{ end }}
Expand Down
25 changes: 0 additions & 25 deletions src/hockeypuck/hkp/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,11 +354,6 @@ func (h *Handler) get(w http.ResponseWriter, l *Lookup) {
return
}

// Drop malformed packets, since these break GPG imports.
for _, key := range keys {
openpgp.DropMalformed(key)
}

w.Header().Set("Content-Type", "application/pgp-keys")
if l.Options[OptionMachineReadable] {
w.Header().Set("Access-Control-Allow-Origin", "*")
Expand Down Expand Up @@ -521,16 +516,6 @@ func (h *Handler) Add(w http.ResponseWriter, r *http.Request, _ httprouter.Param
return
}
for _, key := range keys {
err := openpgp.DropMalformed(key)
if err != nil {
httpError(w, http.StatusInternalServerError, errors.WithStack(err))
return
}
err = openpgp.DropDuplicates(key)
if err != nil {
httpError(w, http.StatusInternalServerError, errors.WithStack(err))
return
}
err = openpgp.ValidSelfSigned(key, false)
if err != nil {
httpError(w, http.StatusInternalServerError, errors.WithStack(err))
Expand Down Expand Up @@ -599,16 +584,6 @@ func (h *Handler) Replace(w http.ResponseWriter, r *http.Request, _ httprouter.P
if signingFp != key.Fingerprint() {
continue
}
err := openpgp.DropMalformed(key)
if err != nil {
httpError(w, http.StatusInternalServerError, errors.WithStack(err))
return
}
err = openpgp.DropDuplicates(key)
if err != nil {
httpError(w, http.StatusInternalServerError, errors.WithStack(err))
return
}
err = openpgp.ValidSelfSigned(key, false)
if err != nil {
httpError(w, http.StatusInternalServerError, errors.WithStack(err))
Expand Down
1 change: 0 additions & 1 deletion src/hockeypuck/hkp/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,6 @@ func (s *HandlerSuite) TestFetchWithBadSigs(c *gc.C) {
keys := openpgp.MustReadArmorKeys(bytes.NewBuffer(armor))
c.Assert(keys, gc.HasLen, 1)
c.Assert(keys[0].ShortID(), gc.Equals, tk.sid)
c.Assert(len(keys[0].Others), gc.Equals, 0)
}

func (s *HandlerSuite) SetupHashQueryTest(c *gc.C, unique bool, digests ...int) (*httptest.ResponseRecorder, *http.Request) {
Expand Down
94 changes: 9 additions & 85 deletions src/hockeypuck/hkp/jsonhkp/jsonhkp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@
package jsonhkp

import (
"encoding/base64"
"fmt"
"io"
"net/url"
"time"

"github.com/pkg/errors"
Expand All @@ -40,9 +37,8 @@ type Packet struct {

func NewPacket(from *openpgp.Packet) *Packet {
return &Packet{
Tag: from.Tag,
Data: from.Packet,
Parsed: from.Parsed,
Tag: from.Tag,
Data: from.Packet,
}
}

Expand All @@ -62,7 +58,6 @@ type PublicKey struct {
Algorithm algorithm `json:"algorithm"`
BitLength int `json:"bitLength"`
Signatures []*Signature `json:"signatures,omitempty"`
Unsupported []*Packet `json:"unsupported,omitempty"`
Packet *Packet `json:"packet,omitempty"`
}

Expand Down Expand Up @@ -94,21 +89,17 @@ func newPublicKey(from *openpgp.PublicKey) *PublicKey {
for _, fromSig := range from.Signatures {
to.Signatures = append(to.Signatures, NewSignature(fromSig))
}
for _, fromPkt := range from.Others {
to.Unsupported = append(to.Unsupported, NewPacket(fromPkt))
}

return to
}

type PrimaryKey struct {
*PublicKey

MD5 string `json:"md5"`
Length int `json:"length"`
SubKeys []*SubKey `json:"subKeys,omitempty"`
UserIDs []*UserID `json:"userIDs,omitempty"`
UserAttrs []*UserAttribute `json:"userAttrs,omitempty"`
MD5 string `json:"md5"`
Length int `json:"length"`
SubKeys []*SubKey `json:"subKeys,omitempty"`
UserIDs []*UserID `json:"userIDs,omitempty"`
}

func NewPrimaryKeys(froms []*openpgp.PrimaryKey) []*PrimaryKey {
Expand All @@ -131,9 +122,6 @@ func NewPrimaryKey(from *openpgp.PrimaryKey) *PrimaryKey {
for _, fromUid := range from.UserIDs {
to.UserIDs = append(to.UserIDs, NewUserID(fromUid))
}
for _, fromUat := range from.UserAttributes {
to.UserAttrs = append(to.UserAttrs, NewUserAttribute(fromUat))
}
return to
}

Expand All @@ -159,10 +147,9 @@ func NewSubKey(from *openpgp.SubKey) *SubKey {
}

type UserID struct {
Keywords string `json:"keywords"`
Packet *Packet `json:"packet,omitempty"`
Signatures []*Signature `json:"signatures,omitempty"`
Unsupported []*Packet `json:"unsupported,omitempty"`
Keywords string `json:"keywords"`
Packet *Packet `json:"packet,omitempty"`
Signatures []*Signature `json:"signatures,omitempty"`
}

func NewUserID(from *openpgp.UserID) *UserID {
Expand All @@ -173,52 +160,9 @@ func NewUserID(from *openpgp.UserID) *UserID {
for _, fromSig := range from.Signatures {
to.Signatures = append(to.Signatures, NewSignature(fromSig))
}
for _, fromPkt := range from.Others {
to.Unsupported = append(to.Unsupported, NewPacket(fromPkt))
}
return to
}

type UserAttribute struct {
Photos []*Photo `json:"photos,omitempty"`
Packet *Packet `json:"packet,omitempty"`
Signatures []*Signature `json:"signatures,omitempty"`
Unsupported []*Packet `json:"unsupported,omitempty"`
}

func NewUserAttribute(from *openpgp.UserAttribute) *UserAttribute {
to := &UserAttribute{
Packet: NewPacket(&from.Packet),
}
for _, image := range from.Images {
to.Photos = append(to.Photos, NewPhoto(image))
}
for _, fromSig := range from.Signatures {
to.Signatures = append(to.Signatures, NewSignature(fromSig))
}
for _, fromPkt := range from.Others {
to.Unsupported = append(to.Unsupported, NewPacket(fromPkt))
}
return to
}

type Photo struct {
MIMEType string `json:"mimeType"`
Contents []byte `json:"contents"`
}

func NewPhoto(image []byte) *Photo {
return &Photo{
MIMEType: "image/jpeg", // The only image format currently supported, AFAIK
Contents: image,
}
}

func (p *Photo) DataURI() (*url.URL, error) {
return url.Parse(fmt.Sprintf(
"data:%s;base64,%s", p.MIMEType, base64.StdEncoding.EncodeToString(p.Contents)))
}

type Signature struct {
SigType int `json:"sigType"`
Revocation bool `json:"revocation,omitempty"`
Expand Down Expand Up @@ -275,9 +219,6 @@ func (pk *PublicKey) packets() []*Packet {
for _, s := range pk.Signatures {
packets = append(packets, s.packets()...)
}
for _, un := range pk.Unsupported {
packets = append(packets, un)
}
return packets
}

Expand All @@ -286,20 +227,6 @@ func (u *UserID) packets() []*Packet {
for _, s := range u.Signatures {
packets = append(packets, s.packets()...)
}
for _, un := range u.Unsupported {
packets = append(packets, un)
}
return packets
}

func (u *UserAttribute) packets() []*Packet {
packets := []*Packet{u.Packet}
for _, s := range u.Signatures {
packets = append(packets, s.packets()...)
}
for _, un := range u.Unsupported {
packets = append(packets, un)
}
return packets
}

Expand All @@ -308,9 +235,6 @@ func (pk *PrimaryKey) packets() []*Packet {
for _, u := range pk.UserIDs {
packets = append(packets, u.packets()...)
}
for _, u := range pk.UserAttrs {
packets = append(packets, u.packets()...)
}
for _, s := range pk.SubKeys {
packets = append(packets, s.packets()...)
}
Expand Down
8 changes: 0 additions & 8 deletions src/hockeypuck/hkp/sks/recon.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,14 +479,6 @@ func (r *Peer) upsertKeys(rcvr *recon.Recover, buf []byte) (*upsertResult, error
}
result := &upsertResult{}
for _, key := range keys {
err := openpgp.DropMalformed(key)
if err != nil {
return nil, errors.WithStack(err)
}
err = openpgp.DropDuplicates(key)
if err != nil {
return nil, errors.WithStack(err)
}
err = openpgp.ValidSelfSigned(key, false)
if err != nil {
return nil, errors.WithStack(err)
Expand Down
8 changes: 0 additions & 8 deletions src/hockeypuck/openpgp/cmd/hashcmp/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,6 @@ func testKeyring(opkr *openpgp.OpaqueKeyring) (int, int, error) {
return 0, 0, errors.WithStack(err)
}

err = openpgp.DropMalformed(pk)
if err != nil {
return 0, 0, errors.WithStack(err)
}
err = openpgp.DropDuplicates(pk)
if err != nil {
return 0, 0, errors.WithStack(err)
}
err = openpgp.ValidSelfSigned(pk, false)
if err != nil {
return 0, 0, errors.WithStack(err)
Expand Down
63 changes: 18 additions & 45 deletions src/hockeypuck/openpgp/io.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (

"github.com/ProtonMail/go-crypto/openpgp"
"github.com/ProtonMail/go-crypto/openpgp/armor"
pgperrors "github.com/ProtonMail/go-crypto/openpgp/errors"
"github.com/ProtonMail/go-crypto/openpgp/packet"
"github.com/pkg/errors"

Expand Down Expand Up @@ -133,8 +132,6 @@ func (ok *OpaqueKeyring) Parse() (*PrimaryKey, error) {
var keyCreationTime time.Time
var length int
for _, opkt := range ok.Packets {
length += len(opkt.Contents)
var badPacket *packet.OpaquePacket
if opkt.Tag == 6 { //packet.PacketTypePublicKey:
if pubkey != nil {
return nil, errors.Errorf("multiple public keys in keyring")
Expand All @@ -151,8 +148,8 @@ func (ok *OpaqueKeyring) Parse() (*PrimaryKey, error) {
signablePacket = nil
subkey, err := ParseSubKey(opkt)
if err != nil {
log.Debugf("unreadable subkey packet in key 0x%s: %v", pubkey.KeyID(), err)
badPacket = opkt
log.Warnf("unreadable subkey packet in key 0x%s: %v", pubkey.Fingerprint(), err)
continue
} else {
pubkey.SubKeys = append(pubkey.SubKeys, subkey)
signablePacket = subkey
Expand All @@ -162,60 +159,40 @@ func (ok *OpaqueKeyring) Parse() (*PrimaryKey, error) {
signablePacket = nil
uid, err := ParseUserID(opkt, pubkey.UUID)
if err != nil {
log.Debugf("unreadable user id packet in key 0x%s: %v", pubkey.KeyID(), err)
badPacket = opkt
log.Warnf("unreadable user id packet in key 0x%s: %v", pubkey.Fingerprint(), err)
continue
} else {
pubkey.UserIDs = append(pubkey.UserIDs, uid)
signablePacket = uid
}
case 17: //packet.PacketTypeUserAttribute:
signablePacket = nil
uat, err := ParseUserAttribute(opkt, pubkey.UUID)
if err != nil {
log.Debugf("unreadable user attribute packet in key 0x%s: %v", pubkey.KeyID(), err)
badPacket = opkt
} else {
pubkey.UserAttributes = append(pubkey.UserAttributes, uat)
signablePacket = uat
}
case 2: //packet.PacketTypeSignature:
if signablePacket == nil {
log.Debugf("signature out of context")
badPacket = opkt
log.Warnf("signature out of context fp=%s", pubkey.Fingerprint())
continue
} else {
sig, err := ParseSignature(opkt, keyCreationTime, pubkey.UUID, signablePacket.uuid())
if err != nil {
log.Debugf("unreadable signature packet in key 0x%s: %v", pubkey.KeyID(), err)
badPacket = opkt
log.Warnf("unreadable signature packet in key 0x%s: %v", pubkey.Fingerprint(), err)
continue
} else {
signablePacket.appendSignature(sig)
}
}
case 10: //packet.PacketTypeMarker:
// drop marker packets, which can appear anywhere without altering the semantics
log.Warnf("marker packet found in OpaqueKeyring")
continue
default:
badPacket = opkt
// make sure that signatures over an unsupported packet are correctly dropped
log.Warnf("unsupported packet type %d in OpaqueKeyring", opkt.Tag)
signablePacket = nil
continue
}

if badPacket != nil {
var badParent string
if signablePacket != nil {
badParent = signablePacket.uuid()
} else {
badParent = pubkey.uuid()
}
other, err := ParseOther(badPacket, badParent)
if err != nil {
return nil, errors.WithStack(err)
}
_, isStructuralError := badPacket.Reason.(pgperrors.StructuralError)
if badPacket.Reason == io.ErrUnexpectedEOF || isStructuralError {
log.Debugf("malformed packet in key 0x%s: %v", pubkey.KeyID(), badPacket.Reason)
other.Malformed = true
}
pubkey.Others = append(pubkey.Others, other)
}
} else if opkt.Tag == 2 { //packet.PacketTypeSignature:
return nil, ErrBareRevocation
}
length += len(opkt.Contents)
}
if pubkey == nil {
return nil, errors.New("primary public key not found")
Expand Down Expand Up @@ -319,7 +296,6 @@ PARSE:
}
current = &OpaqueKeyring{}
current.setPosition(r.r)
currentKeyLen = 0
currentFingerprint = fp
current.Packets = append(current.Packets, op)
case 2:
Expand All @@ -334,10 +310,7 @@ PARSE:
currentFingerprint = ""
current.Packets = append(current.Packets, op)
}
case 13, 14, 17:
//packet.PacketTypeUserId,
//packet.PacketTypeUserAttribute,
//packet.PacketTypePublicSubKey,
default:
if current != nil {
current.Packets = append(current.Packets, op)
}
Expand Down
Loading

0 comments on commit c61a146

Please sign in to comment.