add gomobile

[zerojg]

add mobile

[claude]

libwallet PR #1 Security Review

Summary

• Critical Issues: 5
• High Priority: 1
• Medium Priority: 1
• Suggestions: 2

---

CRITICAL Issues

1. Missing Required Functions and Types

Severity: CRITICAL
Location: gomobile/addresses.go (entire file)

Issue: The file references loadedWallet(), AddressesRes, AddrFromExtKey, CreateExtendedKeyReq, defaultAccount, and ErrCodeNotSynced which are not defined in this file or imported.

Current: References undefined symbols throughout the file
Required: Must define or import:

• func loadedWallet(name string) (*wallet, bool) - with proper mutex locking
• type AddressesRes struct { Used []string; Unused []string; Index uint32 }
• type AddrFromExtKey struct { Key, Path, AddrType string; UseChildBIP32Std bool }
• type CreateExtendedKeyReq struct { Key, ParentKey, ChainCode, Network string; Depth int; ChildN int; IsPrivate bool }
• const defaultAccount = "default"
• const ErrCodeNotSynced = 1

Why: This code will not compile. The gomobile package needs its own implementations separate from the cgo package.

---

2. Password Security Violation

Severity: CRITICAL
Location: gomobile/addresses.go:67

Issue: Password passed as string instead of []byte.

Current:

func SignMessage(name, message, address, password string) (string, error) {
    // ...
    if err := w.MainWallet().Unlock(w.ctx, []byte(password), nil); err != nil {

Suggested:

func SignMessage(name, message, address string, password []byte) (string, error) {
    defer func() {
        for i := range password {
            password[i] = 0
        }
    }()
    if err := w.MainWallet().Unlock(w.ctx, password, nil); err != nil {

Why: Strings cannot be zeroed from memory in Go. Passwords MUST be passed as []byte so they can be securely cleared after use. This is a fundamental security requirement for wallet operations.

Note: Gomobile API restriction - If gomobile cannot accept []byte, the password must be converted immediately and the resulting byte slice zeroed. Document this limitation clearly.

---

3. Extended Key Type Safety

Severity: CRITICAL
Location: gomobile/addresses.go:210-228

Issue: Gomobile doesn't properly support uint8/uint16/uint64. Using type conversions that may cause issues.

Current:

func CreateExtendedKey(createExtKeyJSON string) (string, error) {
    // ...
    extKey, err := dcr.CreateExtendedKey(
        createExt.Key,
        createExt.ParentKey,
        createExt.ChainCode,
        createExt.Network,
        uint8(createExt.Depth),   // Unsafe conversion
        uint32(createExt.ChildN), // Unsafe conversion
        createExt.IsPrivate,
    )

Suggested: Define CreateExtendedKeyReq with int fields:

type CreateExtendedKeyReq struct {
    Key       string `json:"key"`
    ParentKey string `json:"parentkey"`
    ChainCode string `json:"chaincode"`
    Network   string `json:"network"`
    Depth     int    `json:"depth"`      // Changed from uint8
    ChildN    int    `json:"childn"`     // Changed from uint32
    IsPrivate bool   `json:"isprivate"`
}

// Add validation:
if createExt.Depth < 0 || createExt.Depth > 255 {
    return "", fmt.Errorf("depth must be 0-255, got %d", createExt.Depth)
}
if createExt.ChildN < 0 {
    return "", fmt.Errorf("childN must be non-negative, got %d", createExt.ChildN)
}

Why: Gomobile has known issues with uint8/uint16/uint64 types. Use int and validate ranges.

---

HIGH Priority

4. No Mutex Protection for loadedWallet

Severity: HIGH
Location: gomobile/addresses.go (all functions)

Issue: Missing mutex protection when accessing shared wallet map.

Required: The loadedWallet() function MUST implement mutex locking:

var (
    wallets    = make(map[string]*wallet)
    walletsMtx sync.Mutex
)

func loadedWallet(name string) (*wallet, bool) {
    walletsMtx.Lock()
    defer walletsMtx.Unlock()

    w, ok := wallets[name]
    return w, ok
}

Why: Concurrent access to the wallets map without synchronization causes race conditions and potential crashes. Reference: cgo/utils.go:8-18

---

MEDIUM Priority

5. Generic Error Wrapping Missing

Severity: MEDIUM
Location: Multiple locations throughout file

Issue: Errors not wrapped with %w for proper error chain support.

Current: fmt.Errorf("context: %v", err)
Suggested: fmt.Errorf("context: %w", err)

Why: Using %w preserves error chains for errors.Is() and errors.As() checks. This is Go best practice for error handling since Go 1.13.

Locations: Lines 35, 56, 60, 86, 92, 118, 147, 162, 176, 189, 193, 201, 205, 225

---

SUGGESTIONS

6. Inconsistent Error Messages

Severity: SUGGESTION
Location: gomobile/addresses.go:183, 23

Issue: Inconsistent error messages - "does not exist" vs "is not loaded"

Current:

// Line 23
return "", fmt.Errorf("wallet with name %q is not loaded", name)
// Line 185
return "", fmt.Errorf("wallet with name %q does not exist", name)

Suggested: Use consistent terminology. Recommend "is not loaded" throughout.

Why: Consistency improves debugging and user experience.

---

7. Password Error Handling Could Be More Generic

Severity: SUGGESTION
Location: gomobile/addresses.go:85-87

Issue: Wallet unlock errors may leak information about password validity.

Current:

if err := w.MainWallet().Unlock(w.ctx, []byte(password), nil); err != nil {
    return "", fmt.Errorf("cannot unlock wallet: %v", err)
}

Suggested: Consider using a generic error:

if err := w.MainWallet().Unlock(w.ctx, password, nil); err != nil {
    return "", ErrInvalidPassphrase
}

Why: According to security guidelines, crypto errors should be generic to avoid leaking details. However, this may be acceptable for user-facing API where detailed feedback is needed. Consider the threat model.

---

Security Checklist Results

• ✅ Keys/seeds zeroed: Not applicable (no key material directly handled)
• ❓ No secrets in logs: Cannot verify - need to check log statements in referenced code
• ❌ Passphrase as []byte: FAILED - password is string in SignMessage (Issue add gomobile test2 #2)
• ✅ Crypto errors generic: Acceptable for user-facing API
• ✅ Entropy: Not applicable (no random generation in this file)
• ❌ Mutex with defer: MISSING - loadedWallet needs mutex (Issue sync: Add rescan progress. decred/libwallet#4)
• ✅ defer Lock() after Unlock(): CORRECT - properly implemented
• ✅ Context cancellation: Context passed correctly throughout
• ❌ Error wrapping: Missing %w format (Issue sync: Add birth state. decred/libwallet#5)
• ❌ Compilation: WILL NOT COMPILE - missing definitions (Issue add gomobile #1)

---

Action Items

Before this PR can merge:

1. ✅ Define all missing types and constants
2. ✅ Implement loadedWallet() with proper mutex protection
3. ✅ Fix password parameter to use []byte or document gomobile limitation
4. ✅ Fix uint8/uint32 types for gomobile compatibility
5. ✅ Add error wrapping with %w
6. ✅ Verify this code compiles and passes tests
7. ✅ Add tests for all exported functions

---

Additional Notes

This PR appears to be creating gomobile bindings by adapting the cgo implementation. Good approach! However, several critical pieces are missing that exist in the cgo version. Recommend:

1. Create a gomobile/types.go file with all necessary type definitions
2. Create a gomobile/utils.go file with loadedWallet() and wallet map management
3. Consider creating a gomobile/loader.go to mirror cgo's wallet loading functionality
4. Add gomobile-specific documentation about type restrictions (no uint8/uint16/uint64)

Compare with the reference implementation in /cgo/ directory to ensure feature parity.

[claude]

[CRITICAL] gomobile/addresses.go:21 - Missing loadedWallet() function definition

This function is undefined and needs to be implemented with proper mutex protection. See cgo/utils.go:8-18 for reference.

[claude]

[CRITICAL] gomobile/addresses.go:67 - Password Security Violation

Password parameter is string instead of []byte. Strings cannot be zeroed from memory in Go.

Suggested fix:

func SignMessage(name, message, address string, password []byte) (string, error) {
    defer func() {
        for i := range password {
            password[i] = 0
        }
    }()
    if err := w.MainWallet().Unlock(w.ctx, password, nil); err != nil {

If gomobile cannot accept []byte, document this limitation and ensure the password byte slice is zeroed immediately after conversion.

[claude]

[CRITICAL] gomobile/addresses.go:150 - Missing AddressesRes type definition

Required type definition:

type AddressesRes struct {
    Used   []string `json:"used"`
    Unused []string `json:"unused"`
    Index  uint32   `json:"index"`
}

Note: Consider using int instead of uint32 for Index to follow gomobile best practices.

[claude]

[CRITICAL] gomobile/addresses.go:220-221 - Gomobile Type Incompatibility

Gomobile doesn't properly support uint8/uint16/uint64 types. Using conversions like uint8(createExt.Depth) can cause issues.

Suggested fix - Use int and validate:

type CreateExtendedKeyReq struct {
    Key       string `json:"key"`
    ParentKey string `json:"parentkey"`
    ChainCode string `json:"chaincode"`
    Network   string `json:"network"`
    Depth     int    `json:"depth"`      // Use int, not uint8
    ChildN    int    `json:"childn"`     // Use int, not uint32
    IsPrivate bool   `json:"isprivate"`
}

// Add range validation
if createExt.Depth < 0 || createExt.Depth > 255 {
    return "", fmt.Errorf("depth must be 0-255, got %d", createExt.Depth)
}
if createExt.ChildN < 0 {
    return "", fmt.Errorf("childN must be non-negative, got %d", createExt.ChildN)
}

[claude]

[CRITICAL] gomobile/addresses.go:199-203 - Missing AddrFromExtKey and CreateExtendedKeyReq type definitions

These types need to be defined. Suggested:

type AddrFromExtKey struct {
    Key              string `json:"key"`
    Path             string `json:"path"`
    AddrType         string `json:"addrtype"`
    UseChildBIP32Std bool   `json:"usechildbip32std"`
}

type CreateExtendedKeyReq struct {
    Key       string `json:"key"`
    ParentKey string `json:"parentkey"`
    ChainCode string `json:"chaincode"`
    Network   string `json:"network"`
    Depth     int    `json:"depth"`      // Use int for gomobile
    ChildN    int    `json:"childn"`     // Use int for gomobile
    IsPrivate bool   `json:"isprivate"`
}

Recommend creating gomobile/types.go similar to cgo/types.go.

[claude]

[CRITICAL] gomobile/addresses.go:29, 50, 174 - Missing ErrCodeNotSynced and defaultAccount constants

These constants are referenced but not defined:

const (
    ErrCodeNotSynced = 1
)

const defaultAccount = "default"

Recommend creating these in gomobile/types.go or at the top of this file.

[claude]

[HIGH] Multiple locations - Missing error wrapping with %w

Errors should be wrapped with %w instead of %v to preserve error chains for errors.Is() and errors.As().

Current: fmt.Errorf("w.CurrentAddress error: %v", err)
Suggested: fmt.Errorf("w.CurrentAddress error: %w", err)

Affected lines: 35, 56, 60, 86, 92, 118, 147, 162, 176, 189, 193, 201, 205, 225

[claude]

Review Complete ✅

I've completed a comprehensive security review of this PR following the libwallet security guidelines.

Summary

• Total Issues Found: 9
• Critical: 5 issues that prevent compilation or create security vulnerabilities
• High: 1 concurrency issue
• Medium: 1 best practice violation
• Suggestions: 2 improvements

Most Critical Issues to Address First:

1. Missing type definitions and functions - Code will not compile
2. Password security violation - Using string instead of []byte
3. Gomobile type incompatibility - uint8/uint32 issues
4. Missing mutex protection - Race condition risk

Next Steps:

1. Create gomobile/types.go with all type definitions (reference: cgo/types.go)
2. Create gomobile/utils.go with loadedWallet() and mutex protection (reference: cgo/utils.go)
3. Fix password handling for security compliance
4. Update types to use int instead of uint8/uint16/uint64 for gomobile compatibility
5. Add error wrapping with %w
6. Add tests for all exported functions

The detailed review with specific line numbers and code suggestions is in the comments above. Feel free to ask if you need clarification on any of the issues!